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

Jump to bottom

bpo-46615: Don't crash when set operations mutate the sets #31120

Merged
merged 12 commits into from Feb 11, 2022
Merged

bpo-46615: Don't crash when set operations mutate the sets #31120

merged 12 commits into from Feb 11, 2022

Conversation

sweeneyde
Copy link
Member

@sweeneyde sweeneyde commented Feb 4, 2022 •
edited by bedevere-bot

Users of set_next should call Py_INCREF on the resulting set keys to avoid use-after-free.

https://bugs.python.org/issue46615

@sweeneyde sweeneyde requested a review from rhettinger as a code owner Feb 4, 2022
@sweeneyde
Copy link
Member Author

sweeneyde commented Feb 4, 2022 •
edited

Checked for refleaks:

PS C:\Users\sween\Source\Repos\cpython2\cpython> .\python.bat -m test test_set -R3:3                          
Running Debug|x64 interpreter...
0:00:00 Run tests sequentially
0:00:00 [1/1] test_set
beginning 6 repetitions
123456
......

== Tests result: SUCCESS ==

1 test OK.

Total duration: 26.3 sec
Tests result: SUCCESS

tim-one
tim-one reviewed Feb 4, 2022
View changes
Copy link
Member

@tim-one tim-one left a comment

Looks like the right approach to me, but Raymond is much more familiar with this code. Added a comment about something that confused me in the new tests.

Lib/test/test_set.py Outdated Show resolved Hide resolved
erlend-aasland
erlend-aasland reviewed Feb 4, 2022
View changes
Objects/setobject.c Outdated Show resolved Hide resolved
@sweeneyde
Copy link
Member Author

sweeneyde commented Feb 6, 2022

It's unclear to me why this doesn't crash with the added test cases:

cpython/Objects/setobject.c

Lines 905 to 908 in 96b344c

while (_PyDict_Next(other, &pos, &key, &value, &hash)) {
if (set_add_entry(so, key, hash))
return -1;
}

@tim-one
Copy link
Member

tim-one commented Feb 6, 2022

It's unclear to me why this doesn't crash

None of these ever "crashed" on my box (Windows). Instead they weirded out silently in various ways (seemingly infinite loops, or the process just stopped without an error exit code). Just about anything can happen when mucking with freed memory - including no visible symptoms at all. Try running in a mode that checks runtime memory use (like valgrind)?

@sweeneyde
Copy link
Member Author

sweeneyde commented Feb 6, 2022

Aha: set_add_entry already does Py_INCREF(key) internally.

@sweeneyde sweeneyde closed this Feb 9, 2022
@sweeneyde sweeneyde reopened this Feb 9, 2022
JelleZijlstra
JelleZijlstra reviewed Feb 11, 2022
View changes
Lib/test/test_set.py Show resolved Hide resolved
Lib/test/test_set.py
def __eq__(self, other):
if not enabled:
return False
if randrange(20) == 0:
Copy link
Member

@JelleZijlstra JelleZijlstra Feb 11, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Might be better to use something like a counter, so that these tests behave consistently. The current behavior might make the test pass or fail inconsistently depending on what the RNG produces.

Copy link
Member Author

@sweeneyde sweeneyde Feb 11, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm personally okay with randomized tests, though I know others might feel differently. The test should pass for anything the RNG can give, and I'm personally more made more confident in code tested in millions of random situations, which ideally hit all of the corner cases I couldn't think of.

Objects/setobject.c Outdated Show resolved Hide resolved
sweeneyde and others added 3 commits Feb 11, 2022
@sweeneyde @JelleZijlstra
Update Objects/setobject.c
0858513 
Co-authored-by: Jelle Zijlstra <jelle.zijlstra@gmail.com>
@sweeneyde
Re-use key variable
2ab5f4e
@sweeneyde
newline in NEWS entry
c16cba3
serhiy-storchaka
serhiy-storchaka approved these changes Feb 11, 2022
View changes
Copy link
Member

@serhiy-storchaka serhiy-storchaka left a comment

LGTM.

@sweeneyde sweeneyde merged commit 4a66615 into python:main Feb 11, 2022
12 checks passed
@miss-islington
Copy link
Contributor

miss-islington commented Feb 11, 2022

Thanks @sweeneyde for the PR 🌮🎉.. I'm working now to backport this PR to: 3.10.
🐍🍒⛏🤖

@miss-islington
Copy link
Contributor

miss-islington commented Feb 11, 2022

Thanks @sweeneyde for the PR 🌮🎉.. I'm working now to backport this PR to: 3.9.
🐍🍒⛏🤖

@miss-islington
Copy link
Contributor

miss-islington commented Feb 11, 2022

Sorry, @sweeneyde, I could not cleanly backport this to 3.9 due to a conflict.
Please backport using cherry_picker on command line.
cherry_picker 4a66615ba736f84eadf9456bfd5d32a94cccf117 3.9

@bedevere-bot
Copy link

bedevere-bot commented Feb 11, 2022

GH-31284 is a backport of this pull request to the 3.10 branch.

miss-islington pushed a commit to miss-islington/cpython that referenced this pull request Feb 11, 2022
@sweeneyde @miss-islington
bpo-46615: Don't crash when set operations mutate the sets (pythonGH-…
dde3b3e 
…31120)

Ensure strong references are acquired whenever using `set_next()`. Added randomized test cases for `__eq__` methods that sometimes mutate sets when called.
(cherry picked from commit 4a66615)

Co-authored-by: Dennis Sweeney <36520290+sweeneyde@users.noreply.github.com>
miss-islington added a commit that referenced this pull request Feb 11, 2022
@miss-islington @sweeneyde
bpo-46615: Don't crash when set operations mutate the sets (GH-31120)
1f5fe99 
Ensure strong references are acquired whenever using `set_next()`. Added randomized test cases for `__eq__` methods that sometimes mutate sets when called.
(cherry picked from commit 4a66615)

Co-authored-by: Dennis Sweeney <36520290+sweeneyde@users.noreply.github.com>
sweeneyde added a commit to sweeneyde/cpython that referenced this pull request Feb 13, 2022
@sweeneyde
bpo-46615: Don't crash when set operations mutate the sets (pythonGH-…
e3ffd17 
…31120)

Ensure strong references are acquired whenever using `set_next()`. Added randomized test cases for `__eq__` methods that sometimes mutate sets when called.

(cherry picked from commit 4a66615)
@bedevere-bot
Copy link

bedevere-bot commented Feb 13, 2022

GH-31312 is a backport of this pull request to the 3.9 branch.

serhiy-storchaka pushed a commit that referenced this pull request Feb 13, 2022
@sweeneyde
bpo-46615: Don't crash when set operations mutate the sets (GH-31120) (…
c31b8a9 
…GH-31312)

Ensure strong references are acquired whenever using `set_next()`. Added randomized test cases for `__eq__` methods that sometimes mutate sets when called.

(cherry picked from commit 4a66615)
@sweeneyde sweeneyde deleted the set_crashers branch Feb 14, 2022
@sweeneyde sweeneyde mentioned this pull request May 18, 2022
Closed
hello-adam pushed a commit to hello-adam/cpython that referenced this pull request Jun 2, 2022
@sweeneyde @hello-adam
bpo-46615: Don't crash when set operations mutate the sets (pythonGH-…
2a484f1 
…31120) (pythonGH-31312)

Ensure strong references are acquired whenever using `set_next()`. Added randomized test cases for `__eq__` methods that sometimes mutate sets when called.

(cherry picked from commit 4a66615)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@JelleZijlstra JelleZijlstra

@erlend-aasland erlend-aasland

@tim-one tim-one

@serhiy-storchaka serhiy-storchaka

@rhettinger rhettinger

Assignees

@sweeneyde sweeneyde

Labels
type-bug An unexpected behavior, bug, or error
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
8 participants
@sweeneyde @tim-one @miss-islington @bedevere-bot @JelleZijlstra @serhiy-storchaka @erlend-aasland @the-knights-who-say-ni