Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Local variables can be prematurely moved to closure, causing use-after-move #23748

Closed
alex65536 opened this issue Jun 21, 2024 · 2 comments · Fixed by #23769
Closed

Local variables can be prematurely moved to closure, causing use-after-move #23748

alex65536 opened this issue Jun 21, 2024 · 2 comments · Fixed by #23769

Comments

@alex65536
Copy link
Contributor

Description

Reproducer:

type
  O = ref object
    s: string
    cb: seq[proc()]

proc push1(o: O, i: int) =
  let o = o
  echo o.s, " ", i
  o.cb.add(proc() = echo o.s, " ", i)

proc push2(o: O, i: int) =
  let o = o
  echo o.s, " ", i
  proc p() = echo o.s, " ", i
  o.cb.add(p)

let o = O(s: "hello", cb: @[])
o.push1(42)  # This segfaults
o.push2(42)  # This also segfaults

This code segfaults under --mm:orc and --mm:arc, but works under --mm:refc.

Nim Version

Reproduces on devel, 2.0.6 and 1.6.16 with --mm:orc, and doesn't reproduce with --mm:refc.

Current Output

hello 42
SIGSEGV: Illegal storage access. (Attempt to read from nil?)
Segmentation fault

Expected Output

hello 42
hello 42

Possible Solution

No response

Additional Information

--expandArc:push2 gives the following:

var
  :env
  :tmpD
try:
  :env = [type node]()
  :env.i2 = i
  `=copy`(:env.o1, o, true)
  echo [:env.o1.s, " ",
    :tmpD = `$`(:env.i2)
    :tmpD]
  proc p(:envP) =
    echo [`$`(o_1.s), " ", `$`(i)]
  
  add(:env.o1.cb, (p,
    let blitTmp = :env
    nimMarkCyclic(:env)
    `=wasMoved`(:env)
    blitTmp))
finally:
  `=destroy`(:tmpD)
  `=destroy_1`(:env)

It can be seen that using :env.o1.cb happens on the same line with moving :env into blitTmp.

The generated C code for the fragment

  add(:env.o1.cb, (p,
    let blitTmp = :env
    nimMarkCyclic(:env)
    `=wasMoved`(:env)
    blitTmp))

looks as follows:

	nimln_(9);	nimln_(7);	blitTmp = colonenv_;
	nimMarkCyclic(colonenv_);
	nimln_(6);	eqwasMoved___test_u243(&colonenv_);
	if (NIM_UNLIKELY(*nimErr_)) goto LA1_;
	nimZeroMem((void*)(&T4_), sizeof(tyProc__HzVCwACFYM9cx9aV62PdjtuA));
	T4_.ClP_0 = colonanonymous___test_u10; T4_.ClE_0 = blitTmp;
	nimln_(9);	add__test_u48((&(*(*colonenv_).o1).cb), T4_);

Here, is it obvious that :env is moved first and then reused, thus leading to use-after-move.

@alex65536
Copy link
Contributor Author

!nim c

type
  O = ref object
    s: string
    cb: seq[proc()]

proc push1(o: O, i: int) =
  let o = o
  echo o.s, " ", i
  o.cb.add(proc() = echo o.s, " ", i)

proc push2(o: O, i: int) =
  let o = o
  echo o.s, " ", i
  proc p() = echo o.s, " ", i
  o.cb.add(p)

let o = O(s: "hello", cb: @[])
o.push1(42)  # This segfaults
o.push2(42)  # This also segfaults

@alex65536
Copy link
Contributor Author

By the way, workaround is simple here: just add

discard o

in the end of the proc and everything will work fine.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

1 participant