Skip to content

/cpython

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

bpo-38588: Fix possible crashes in dict and list when calling PyObject_RichCompareBool #17734

Merged
merged 4 commits into from Dec 31, 2019
Merged

bpo-38588: Fix possible crashes in dict and list when calling PyObject_RichCompareBool #17734

merged 4 commits into from Dec 31, 2019
+47 −1

Conversation

@corona10
Copy link
Member

corona10 commented Dec 29, 2019 •
edited by bedevere-bot

Based on LCatro's report

https://bugs.python.org/issue38588
@corona10
bpo-38588: Fix segfaults when dict comparision with modifying operand
Loading status checks…
4aaa382
@corona10 corona10 requested a review from methane as a code owner Dec 29, 2019
@corona10 corona10 changed the title bpo-38588: Fix segfaults when dict comparision with modifying operand bpo-38588: Fix segfaults when dict comparison with modifying operand Dec 29, 2019
@pablogsal

This comment has been minimized.

Sign in to view
Copy link
Member

pablogsal commented Dec 29, 2019

Could you include the rest of the cases that LCatro was mentioning? He mentioned another 2 for lists.
@rhettinger
rhettinger approved these changes Dec 29, 2019
View changes
@corona10

This comment has been minimized.

Sign in to view
Copy link
Member Author

corona10 commented Dec 30, 2019 •
edited

@pablogsal

poc3 is not reproducible on my local mac machine and Linux machine with the master branch.
Looks like the master branch code was changed

cpython/Objects/listobject.c

Line 452 in 3c87a66

cmp = PyObject_RichCompareBool(PyList_GET_ITEM(a, i), el, Py_EQ);

However poc2 is reproducible, so I will add the patch on this PR.

$ cat poc3.py
class poc() :
    def __eq__(self,other) :
        list1.clear()
        return NotImplemented

list1 = [ set() ]
poc() in list1
print('end')

$ ./python poc3.py
end
@ZackerySpytz

This comment has been minimized.

Sign in to view
Copy link
Contributor

ZackerySpytz commented Dec 30, 2019

FWIW, LCatro had opened another issue (https://bugs.python.org/issue38610) for similar crashes in the index(), count(), and remove() methods of list. I had created GH-17022 to address them.
@corona10
bpo-38588: Fix list case
Loading status checks…
035a9d7
@corona10

This comment has been minimized.

Sign in to view
Copy link
Member Author

corona10 commented Dec 30, 2019 •
edited

@pablogsal
Please take a look,
I 've added the list case patch except which is not reproducible in the master branch.
(I've attached the script above)

And for the list_richcompare I've added more cases that were not reported by LCatro.
Both cases are added in the unit test and fixed.

And IMHO, we can close bpo-38588 with this patch
:)
@corona10
bpo-38588: Add comment
Loading status checks…
e809f7a
@corona10 corona10 requested a review from pablogsal Dec 30, 2019
@corona10 corona10 changed the title bpo-38588: Fix segfaults when dict comparison with modifying operand bpo-38588: Fix possible crashes in dict and list when calling PyObject_RichCompareBool Dec 30, 2019
@pablogsal
pablogsal reviewed Dec 30, 2019
View changes
Lib/test/test_list.py Outdated Show resolved Hide resolved
@pablogsal
pablogsal reviewed Dec 30, 2019
View changes
Lib/test/test_dict.py Outdated Show resolved Hide resolved
@corona10
bpo-38588: Update comment
Loading status checks…
645e1fb
@corona10

This comment has been minimized.

Sign in to view
Copy link
Member Author

corona10 commented Dec 31, 2019

@pablogsal Updated!
@pablogsal
pablogsal approved these changes Dec 31, 2019
View changes
Copy link
Member

pablogsal left a comment

LGTM

Thanks for the quick fix, @corona10!
@pablogsal pablogsal merged commit 2d5bf56 into python:master Dec 31, 2019
9 checks passed
9 checks passed
Docs
Details
Windows (x86)
Details
Windows (x64)
Details
macOS
Details
Ubuntu
Details
Azure Pipelines PR #20191231.1 succeeded
Details
bedevere/issue-number Issue number 38588 found
Details
bedevere/news News entry found in Misc/NEWS.d
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@miss-islington

This comment has been minimized.

Sign in to view
Copy link

miss-islington commented Dec 31, 2019

Thanks @corona10 for the PR, and @pablogsal for merging it 🌮🎉.. I'm working now to backport this PR to: 3.7, 3.8.
🐍🍒⛏🤖
@miss-islington

This comment has been minimized.

Sign in to view
Copy link

miss-islington commented Dec 31, 2019

I'm having trouble backporting to 3.8. Reason: 'Error 110 while writing to socket. Connection timed out.'. Please retry by removing and re-adding the needs backport to 3.8 label.
@miss-islington

This comment has been minimized.

Sign in to view
Copy link

miss-islington commented Dec 31, 2019

Sorry, @corona10 and @pablogsal, I could not cleanly backport this to 3.7 due to a conflict.
Please backport using cherry_picker on command line.
cherry_picker 2d5bf568eaa5059402ccce9ba5a366986ba27c8a 3.7
@pablogsal

This comment has been minimized.

Sign in to view
Copy link
Member

pablogsal commented Dec 31, 2019 •
edited

@corona10 Can you make the backport to 3.7 and 3.8 using cherry_picker?
@miss-islington

This comment has been minimized.

Sign in to view
Copy link

miss-islington commented Dec 31, 2019

Thanks @corona10 for the PR, and @pablogsal for merging it 🌮🎉.. I'm working now to backport this PR to: 3.8.
🐍🍒⛏🤖
@miss-islington

This comment has been minimized.

Sign in to view
Copy link

miss-islington commented Dec 31, 2019

Sorry @corona10 and @pablogsal, I had trouble checking out the 3.8 backport branch.
Please backport using cherry_picker on command line.
cherry_picker 2d5bf568eaa5059402ccce9ba5a366986ba27c8a 3.8
@corona10 corona10 mentioned this pull request Dec 31, 2019
Merged
@bedevere-bot

This comment has been minimized.

Sign in to view
Copy link

bedevere-bot commented Dec 31, 2019

GH-17764 is a backport of this pull request to the 3.8 branch.
corona10 added a commit to corona10/cpython that referenced this pull request Dec 31, 2019
@corona10
[3.8] bpo-38588: Fix possible crashes in dict and list when calling P…
6a341a7 
…yObject_RichCompareBool (pythonGH-17734)

Take strong references before calling PyObject_RichCompareBool to protect against the case
where the object dies during the call.
(cherry picked from commit 2d5bf56)

Co-authored-by: Dong-hee Na <donghee.na92@gmail.com>
@methane

This comment has been minimized.

Sign in to view
Copy link
Member

methane commented Dec 31, 2019

Oh, please check the b.p.o before merge...
@pablogsal

This comment has been minimized.

Sign in to view
Copy link
Member

pablogsal commented Dec 31, 2019

Oh, please check the b.p.o before merge...

Apologies, I had totally missed the comment in bpo :(
@corona10 corona10 mentioned this pull request Dec 31, 2019
Merged
@bedevere-bot

This comment has been minimized.

Sign in to view
Copy link

bedevere-bot commented Dec 31, 2019

GH-17765 is a backport of this pull request to the 3.7 branch.
@pablogsal

This comment has been minimized.

Sign in to view
Copy link
Member

pablogsal commented Dec 31, 2019

Sorry @corona10, I will leave the backports open until we decide what to do finally in the bpo issue.
@corona10

This comment has been minimized.

Sign in to view
Copy link
Member Author

corona10 commented Dec 31, 2019

@pablogsal cc @methane
Yes, that should be done first :)
pablogsal added a commit that referenced this pull request Dec 31, 2019
@corona10 @methane @pablogsal
[3.7] bpo-38588: Fix possible crashes in dict and list when calling P… (
Loading status checks…
53f11ba 
GH-17765)

* [3.7] bpo-38588: Fix possible crashes in dict and list when calling PyObject_RichCompareBool (GH-17734)

Take strong references before calling PyObject_RichCompareBool to protect against the case
where the object dies during the call..
(cherry picked from commit 2d5bf56)

Co-authored-by: Dong-hee Na <donghee.na92@gmail.com>

* methane's suggestion

methane's suggestion

Co-Authored-By: Inada Naoki <songofacandy@gmail.com>

Co-authored-by: Inada Naoki <songofacandy@gmail.com>
pablogsal added a commit that referenced this pull request Dec 31, 2019
@corona10 @methane @pablogsal
[3.8] bpo-38588: Fix possible crashes in dict and list when calling P… (
Loading status checks…
2ee8791 
GH-17764)

* [3.8] bpo-38588: Fix possible crashes in dict and list when calling PyObject_RichCompareBool (GH-17734)

Take strong references before calling PyObject_RichCompareBool to protect against the case
where the object dies during the call.
(cherry picked from commit 2d5bf56)

Co-authored-by: Dong-hee Na <donghee.na92@gmail.com>

* Update Objects/listobject.c

@methane's suggestion

Co-Authored-By: Inada Naoki <songofacandy@gmail.com>

Co-authored-by: Inada Naoki <songofacandy@gmail.com>
@corona10 corona10 deleted the corona10:bpo-38588 branch Dec 31, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@rhettinger rhettinger

@pablogsal pablogsal

@methane methane

Assignees

@pablogsal pablogsal

Labels
CLA signed
Projects
None yet
Milestone
No milestone
8 participants
@corona10 @pablogsal @ZackerySpytz @miss-islington @bedevere-bot @methane @rhettinger @the-knights-who-say-ni
You can’t perform that action at this time.