GV

GeezerVison

The World Wide Web, restored to 800 pixels of glory.

Home|About|Gaming cabinet|Archives|How it works|Starter sites

On this site

  • Sign in
  • ActionsCI/CD and automation
  • CodespacesCloud development environments
  • All Features
  • Documentation
  • Training
  • Certifications
  • Build with Copilot
  • Customize Copilot
  • Integrations
  • Trending repos
  • GitHub Sponsors
  • Developer articles
  • Enterprises

This view

  • Original page
  • New address

GeezerVison is showing github.com — bpo-46615: Don't crash when set operations mutate the sets by sweeneyde · Pull Request #31120 · python/cpython

Conversation

@sweeneyde

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

Uh oh!

There was an error while loading. Please reload this page.

Copy link
Copy Markdown
Member

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

https://bugs.python.org/issue46615

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
@sweeneyde
Don't crash when set operations mutate the sets
8f858b0
@sweeneyde
sweeneyde requested a review from rhettinger as a code owner February 4, 2022 04:24
@the-knights-who-say-ni the-knights-who-say-ni added the CLA signed label Feb 4, 2022
@bedevere-bot bedevere-bot added the awaiting review label Feb 4, 2022
blurb-it Bot and others added 2 commits February 4, 2022 04:33
@blurb-it
📜🤖 Added by blurb_it.
ba679c2
@sweeneyde
make new test, don't repeat ops on empty sets
780bca9
@sweeneyde

sweeneyde commented Feb 4, 2022 •
edited
Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link
Copy Markdown
Member Author

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
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
sweeneyde added 2 commits February 4, 2022 00:06
@sweeneyde
Paramerize tests by type
16e990d
@sweeneyde
test xor as well
0f42bb8
tim-one
tim-one reviewed Feb 4, 2022
View reviewed changes

@tim-one tim-one left a comment

Copy link
Copy Markdown
Member

There was a problem hiding this 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.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
Comment thread Lib/test/test_set.py Resolved

Uh oh!

There was an error while loading. Please reload this page.
sweeneyde added 2 commits February 4, 2022 01:22
@sweeneyde
Catch RuntimeError explicitly. Only test methods that are actually pr…
c83c426 
…esent (no TypeErrors)
@sweeneyde
More consistent naming of TestCase classes
a8951b0
erlend-aasland
erlend-aasland reviewed Feb 4, 2022
View reviewed changes
Comment thread Objects/setobject.c Resolved

Uh oh!

There was an error while loading. Please reload this page.
erlend-aasland
erlend-aasland reviewed Feb 4, 2022
View reviewed changes
Comment thread Objects/setobject.c Resolved

Uh oh!

There was an error while loading. Please reload this page.
erlend-aasland
erlend-aasland reviewed Feb 4, 2022
View reviewed changes
Comment thread Objects/setobject.c Resolved

Uh oh!

There was an error while loading. Please reload this page.
sweeneyde added 2 commits February 4, 2022 05:56
@sweeneyde
Factor out a common DECREF
44e40c2
@sweeneyde
Factor out another common DECREF
b4091fd
@sweeneyde

sweeneyde commented Feb 6, 2022

Copy link
Copy Markdown
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;
}

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@tim-one

tim-one commented Feb 6, 2022

Copy link
Copy Markdown
Member

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)?
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@sweeneyde

sweeneyde commented Feb 6, 2022

Copy link
Copy Markdown
Member Author

Aha: set_add_entry already does Py_INCREF(key) internally.
tim-one and erlend-aasland reacted with thumbs up emoji
All reactions
  • 👍 2 reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@sweeneyde sweeneyde closed this Feb 9, 2022
@sweeneyde sweeneyde reopened this Feb 9, 2022
JelleZijlstra
JelleZijlstra reviewed Feb 11, 2022
View reviewed changes
Comment thread Lib/test/test_set.py Resolved

Uh oh!

There was an error while loading. Please reload this page.
Comment thread Lib/test/test_set.py
def __eq__(self, other):
if not enabled:
return False
if randrange(20) == 0:

@JelleZijlstra JelleZijlstra Feb 11, 2022

Copy link
Copy Markdown
Member

There was a problem hiding this comment.

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.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions

@sweeneyde sweeneyde Feb 11, 2022

Copy link
Copy Markdown
Member Author

There was a problem hiding this comment.

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.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
serhiy-storchaka reacted with thumbs up emoji
All reactions
  • 👍 1 reaction
Comment thread Objects/setobject.c Resolved

Uh oh!

There was an error while loading. Please reload this page.
sweeneyde and others added 3 commits February 10, 2022 21:22
@sweeneyde @JelleZijlstra
Update Objects/setobject.c
0858513 
Co-authored-by: Jelle Zijlstra <[email protected]>
@sweeneyde
Re-use key variable
2ab5f4e
@sweeneyde
newline in NEWS entry
c16cba3
serhiy-storchaka
serhiy-storchaka approved these changes Feb 11, 2022
View reviewed changes

@serhiy-storchaka serhiy-storchaka left a comment

Copy link
Copy Markdown
Member

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

LGTM.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
@bedevere-bot bedevere-bot added awaiting merge and removed awaiting review labels Feb 11, 2022
@sweeneyde
sweeneyde merged commit 4a66615 into python:main Feb 11, 2022
@bedevere-bot bedevere-bot removed the awaiting merge label Feb 11, 2022
@serhiy-storchaka serhiy-storchaka added needs backport to 3.9 type-bug An unexpected behavior, bug, or error labels Feb 11, 2022
@miss-islington

miss-islington commented Feb 11, 2022

Copy link
Copy Markdown
Contributor

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@miss-islington

miss-islington commented Feb 11, 2022

Copy link
Copy Markdown
Contributor

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@miss-islington

miss-islington commented Feb 11, 2022

Copy link
Copy Markdown
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
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@miss-islington miss-islington assigned sweeneyde Feb 11, 2022
@bedevere-bot bedevere-bot removed the needs backport to 3.10 label Feb 11, 2022
@bedevere-bot

bedevere-bot commented Feb 11, 2022

Copy link
Copy Markdown

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
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

bedevere-bot commented Feb 13, 2022

Copy link
Copy Markdown

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@bedevere-bot bedevere-bot removed the needs backport to 3.9 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
Use-after-free by mutating set during set operations #90773
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
@serhiy-storchaka serhiy-storchaka serhiy-storchaka approved these changes
@rhettinger rhettinger Awaiting requested review from rhettinger rhettinger is a code owner
+1 more reviewer
@tim-one tim-one tim-one left review comments
Reviewers whose approvals may not affect merge requirements

Uh oh!

There was an error while loading. Please reload this page.

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.

Uh oh!

There was an error while loading. Please reload this page.

8 participants

@sweeneyde @tim-one @miss-islington @bedevere-bot @JelleZijlstra @serhiy-storchaka @erlend-aasland @the-knights-who-say-ni
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
You are visitor 01402503 — GeezerVison does not store the pages you look up.
Best viewed with Netscape Navigator 4.7 or Internet Explorer 6 at 800×600. No cookies required.