Skip to content

Commit

Permalink
fix(typeinfo): expanding seq doesn't clear locations
Browse files Browse the repository at this point in the history
Summary
=======

New seq slots are now zeroed when expanding the seq via `invokeNewSeq`
or `extendSeq`, making the behaviour consistent with `setLen` and
`newSeq`, and also fixing crashes with `marshal` caused by the
uninitialized memory.

Details
=======

Zeroing the memory is not correct for types that don't have a zero-
default, but those cannot be detected with just RTTI. Zeroing the
memory is usually still better then leaving it as is.

For the JavaScript and VM backends, the zeroMem call is excluded from
compilation. Using `invokeNewSeq` and `extendSeq` is already not
possible on these backends.
  • Loading branch information
zerbina committed Sep 25, 2024
1 parent 3c71f44 commit 2e79550
Show file tree
Hide file tree
Showing 2 changed files with 29 additions and 3 deletions.
18 changes: 15 additions & 3 deletions lib/core/typeinfo.nim
Original file line number Diff line number Diff line change
Expand Up @@ -108,6 +108,9 @@ proc prepareSeqAdd(len: int; p: pointer; addlen, elemSize, elemAlign: int): poin

template `+!!`(a, b): untyped = cast[pointer](cast[ByteAddress](a) + b)

proc align(address, alignment: int): int =
result = (address + (alignment - 1)) and not (alignment - 1)

proc getDiscriminant(aa: pointer, n: ptr TNimNode): int =
assert(n.kind == nkCase)
var d: int
Expand Down Expand Up @@ -180,6 +183,11 @@ proc invokeNewSeq*(x: Any, len: int) =
s.len = len
let elem = x.rawType.base
s.p = cast[ptr NimSeqPayloadReimpl](newSeqPayload(len, elem.size, elem.align))
if len > 0:
# zeroing the memory might not be legal depending on the type, but there's
# curently no way to check that here
when declared(zeroMem): # not the case for JS and the VM
zeroMem(s.p +!! align(sizeof(int), elem.align), elem.size * len)

proc extendSeq*(x: Any) =
## Performs `setLen(x, x.len+1)`. `x` needs to represent a `seq`.
Expand All @@ -189,6 +197,13 @@ proc extendSeq*(x: Any) =
let elem = x.rawType.base
if s.p == nil or s.p.cap < s.len+1:
s.p = cast[ptr NimSeqPayloadReimpl](prepareSeqAdd(s.len, s.p, 1, elem.size, elem.align))

# zeroing the memory might not be legal depending on the type, but there's
# curently no way to check that here
when declared(zeroMem): # not the case for JS and the VM
let headerSize = align(sizeof(int), elem.align)
zeroMem(s.p +!! (headerSize + s.len * elem.size), elem.size)

inc s.len

proc setObjectRuntimeType*(x: Any) =
Expand All @@ -203,9 +218,6 @@ proc skipRange(x: PNimType): PNimType {.inline.} =
result = x
if result.kind == tyRange: result = result.base

proc align(address, alignment: int): int =
result = (address + (alignment - 1)) and not (alignment - 1)

proc `[]`*(x: Any, i: int): Any =
## Accessor for an any `x` that represents an array or a sequence.
case x.rawType.kind
Expand Down
14 changes: 14 additions & 0 deletions tests/stdlib/metaprogramming/ttypeinfo.nim
Original file line number Diff line number Diff line change
Expand Up @@ -69,3 +69,17 @@ block:

doAssert getEnumOrdinal(y, "Hello") == 0
doAssert getEnumOrdinal(y, "hello") == 1

block seq_extension_zeroes:
# make sure ``invokeNewSeq`` and ``extendSeq`` zero the new locations
var s: seq[int]
toAny(s).invokeNewSeq(100)

# check that the values are all zero and change them to something else
for it in s.mitems:
doAssert it == 0
it = 1

s.setLen(0) # clear the seq, which should leave the memory untouched
toAny(s).extendSeq() # grow by one
doAssert s[0] == 0

0 comments on commit 2e79550

Please sign in to comment.