Skip to content

Commit

Permalink
fix(typeinfo): expanding seq doesn't clear locations (#1463)
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.

Fixes #1462
  • Loading branch information
zerbina authored Sep 25, 2024
1 parent 3c71f44 commit c61e95b
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 c61e95b

Please sign in to comment.