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

Iteration safety for containers #2162

Closed
davidhewitt opened this issue Feb 12, 2022 · 2 comments · Fixed by #2380
Closed

Iteration safety for containers #2162

davidhewitt opened this issue Feb 12, 2022 · 2 comments · Fixed by #2380
Assignees
Labels

Comments

@davidhewitt
Copy link
Member

The other day I noticed this doc comment on PyDict::iter.

    /// Note that it's unsafe to use when the dictionary might be changed by
    /// other code.

This sounds scary; it's a safe function so shouldn't have arbitrary safety warnings like this.

I know that when iterating Python containers from Python they do have checks to prevent modification. We should probably check for our container types what happens if we try to modify during iteration, and also whether it really is unsound or just might introduce logical bugs.

I'd argue that if our current implementation is indeed unsound then I'd hope we can patch things up to prevent modification so that PyDict::iter and friends can remain safe.

(We should only need to check PyList, PyDict, and PySet; PyFrozenSet and PyTuple are immutable so shouldn't need this protection).

@nw0
Copy link
Contributor

nw0 commented Mar 17, 2022

The iterator code only uses PyDict_Next (C-API doc), which says

The dictionary p should not be mutated during iteration. It is safe to modify the values of the keys as you iterate over the dictionary, but only so long as the set of keys does not change. (emphasis mine)

The phrasing is a bit ambiguous, so I read this statement as:

  1. Don't add or remove keys from the dictionary
  2. Don't modify the keys (which you shouldn't be doing anyway...)

I don't think we can add/remove keys on the (safe) Rust side as long as we hold the iterator. Is it possible to violate this by calling Python code?

Also, as I understand it (possibly incorrectly) the returned &'py PyAny could be used to mutate the underlying Python object -- in this case, the key. Arguably Python programmers already know they shouldn't do this: I'm not sure whether we can actually do something about that given #883.

@birkenfeld
Copy link
Member

You can do anything to the dict while the iterator is active since mutating operations are fine with shared borrows.

So it seems like to be safe here, we cannot use the C-layer iterators, but have to use the Python-layer iterators, which need to guard against all the same things that we do - and so e.g. raise RuntimeError when the dictionary changes during iteration.

Since PyDict_Next is probably a lot faster, we can still offer it as unsafe API, with the necessary warnings.

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

Successfully merging a pull request may close this issue.

4 participants