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.

Sign up for GitHub

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
Loading

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 February 4, 2022 04:24
@sweeneyde
Copy link
Member Author

sweeneyde commented Feb 4, 2022 •
edited
Loading

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 reviewed changes
Copy link
Member

@tim-one tim-one left a comment

Choose a reason for hiding this comment

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

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.

erlend-aasland
erlend-aasland reviewed Feb 4, 2022
View reviewed changes
@sweeneyde
Copy link
Member Author

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

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 reviewed changes
Lib/test/test_set.py
def __eq__(self, other):
if not enabled:
return False
if randrange(20) == 0:
Copy link
Member

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

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.

serhiy-storchaka
serhiy-storchaka approved these changes Feb 11, 2022
View reviewed changes
Copy link
Member

@serhiy-storchaka serhiy-storchaka left a comment

Choose a reason for hiding this comment

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

LGTM.

@sweeneyde sweeneyde merged commit 4a66615 into python:main Feb 11, 2022
@serhiy-storchaka serhiy-storchaka added needs backport to 3.9 only security fixes needs backport to 3.10 only security fixes type-bug An unexpected behavior, bug, or error labels Feb 11, 2022
@miss-islington
Copy link
Contributor

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

@miss-islington
Copy link
Contributor

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

@miss-islington
Copy link
Contributor

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 bedevere-bot removed the needs backport to 3.10 only security fixes label Feb 11, 2022
@bedevere-bot
Copy link

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 <[email protected]>
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 <[email protected]>
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

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

@bedevere-bot bedevere-bot removed the needs backport to 3.9 only security fixes label Feb 13, 2022
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 February 14, 2022 22:20
@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 JelleZijlstra left review comments

@erlend-aasland erlend-aasland erlend-aasland left review comments

@tim-one tim-one tim-one left review comments

@serhiy-storchaka serhiy-storchaka serhiy-storchaka approved these changes

@rhettinger rhettinger Awaiting requested review from rhettinger rhettinger is a code owner

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.
8 participants
@sweeneyde @tim-one @miss-islington @bedevere-bot @JelleZijlstra @serhiy-storchaka @erlend-aasland @the-knights-who-say-ni