Skip to content

fixes #24806; generates =wasMoved for seqs self-assignment to reset the buffer #24830

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

Closed
wants to merge 1 commit into from

Conversation

ringabout
Copy link
Member

@ringabout ringabout commented Mar 31, 2025

fixes #24806

In the case of self assignments, x.p is the same as y.p. We cannot modify x.p without changing y.p as well. We need to reset x.p to nil so that setLen will initialize x with uninit values. Now it's okay to copy values of y.p

proc `=copy`(x, y: seq) =
  if x.p == y.p:
    wasMoved(x)
  setLen(x, y.len)
  var i = 0
  while i < y.len: `=copy`(x[i], y[i]); inc(i)

@ringabout ringabout changed the title fixes #24806; =wasMoved for seqs reset self assignment x.p fixes #24806; =wasMoved for seqs self-assignment to resetx.p Mar 31, 2025
@ringabout ringabout changed the title fixes #24806; =wasMoved for seqs self-assignment to resetx.p fixes #24806; =wasMoved for seqs self-assignment to reset the buffer Mar 31, 2025
@ringabout ringabout changed the title fixes #24806; =wasMoved for seqs self-assignment to reset the buffer fixes #24806; generates =wasMoved for seqs self-assignment to reset the buffer Mar 31, 2025
@zerbina
Copy link
Contributor

zerbina commented Apr 1, 2025

This change will lead to a memory leak in the following case:

proc test() =
  var s = newSeq[int](1)
  let dst = addr s
  let src = addr s
  # `dst` and `src` point to the same location
  dst[] = src[]
  # expands to `=copy(dst[], src[])`

test()

The wasMoved(x) within the =copy will zero the payload pointer (while leaving the payload allocated), with the subsequent setLen then allocating a new payload and storing it in x.

Once control-flow leaves =copy, the temporary stack location still storing the old payload address stops (logically) existing and the old payload becomes unreachable -> memory leak.

@ringabout
Copy link
Member Author

Make sense, thank you!

@ringabout ringabout closed this Apr 1, 2025
@ringabout ringabout deleted the pr_copy_seq branch April 1, 2025 03:36
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 this pull request may close these issues.

weird SIGSEV with scopes and seqs
2 participants