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-38395: Fix ownership in weakref.proxy methods #16632

Merged
merged 6 commits into from Oct 8, 2019
Merged

bpo-38395: Fix ownership in weakref.proxy methods #16632

merged 6 commits into from Oct 8, 2019

Conversation

pablogsal
Copy link
Member

@pablogsal pablogsal commented Oct 8, 2019 •
edited by bedevere-bot

The implementation of weakref.proxy's methods call back into the Python
API using a borrowed references of the weakly referenced object
(acquired via PyWeakref_GET_OBJECT). This API call may delete the last
reference to the object (either directly or via GC), leaving a dangling
pointer, which can be subsequently dereferenced.

To fix this, claim temporarily ownership of the referenced object when
calling the appropiate method. Some functions because at the moment they
do not need to access the borrowed referent, but to protect against
future changes to these functions, ownership needs to be fixed in
all potentially affected methids.

https://bugs.python.org/issue38395

@pablogsal
bpo-38395: Fix ownership in weakref.proxy methods
4ad5626 
The implementation of weakref.proxy's methods call back into the Python
API using a borrowed references of the weakly referenced object
(acquired via PyWeakref_GET_OBJECT). This API call may delete the last
reference to the object (either directly or via GC), leaving a dangling
pointer, which can be subsequently dereferenced.

To fix this, claim temporarily ownership of the referenced object when
calling the appropiate method. Some functions because at the moment they
do not need to access the borrowed referent, but to protect against
future changes to these functions, ownership needs to be fixed in
all potentially affected methids.
@pablogsal pablogsal requested a review from vstinner Oct 8, 2019
@pablogsal pablogsal requested a review from nascheme Oct 8, 2019
@pablogsal pablogsal self-assigned this Oct 8, 2019
@pablogsal pablogsal requested a review from pitrou Oct 8, 2019
pitrou
pitrou approved these changes Oct 8, 2019
View changes
Copy link
Member

@pitrou pitrou left a comment

LGTM, thank you

vstinner
vstinner reviewed Oct 8, 2019
View changes
Copy link
Member

@vstinner vstinner left a comment

weakref_hash() doesn't seem to hold a strong reference to call PyObject_Hash() which may execute arbitrary Python code, no?

Same remark for weakref_repr(), weakref_richcompare(), proxy_bool().

Objects/weakrefobject.c Show resolved Hide resolved
Lib/test/test_weakref.py Show resolved Hide resolved
@pablogsal
Copy link
Member Author

pablogsal commented Oct 8, 2019

@vstinner Done, can you review again?

vstinner
vstinner reviewed Oct 8, 2019
View changes
Copy link
Member

@vstinner vstinner left a comment

Hum, WRAP_UNARY(), WRAP_BINARY(), WRAP_TERNARY() and WRAP_METHOD() still have the same bug, no?

By the way, UNWRAP_I() is no longer used and so can be removed.

Objects/weakrefobject.c Outdated Show resolved Hide resolved
Objects/weakrefobject.c Outdated Show resolved Hide resolved
vstinner
vstinner approved these changes Oct 8, 2019
View changes
Copy link
Member

@vstinner vstinner left a comment

LGTM. Goodbye borrowed references!

@pablogsal pablogsal merged commit 10cd00a into python:master Oct 8, 2019
@pablogsal pablogsal deleted the bpo-38395 branch Oct 8, 2019
@miss-islington
Copy link
Contributor

miss-islington commented Oct 8, 2019

Thanks @pablogsal for the PR 🌮🎉.. I'm working now to backport this PR to: 3.6, 3.7, 3.8.
🐍🍒⛏🤖

@miss-islington
Copy link
Contributor

miss-islington commented Oct 8, 2019

Sorry, @pablogsal, I could not cleanly backport this to 3.8 due to a conflict.
Please backport using cherry_picker on command line.
cherry_picker 10cd00a9e3c22af37c748ea5a417f6fb66601e21 3.8

@miss-islington
Copy link
Contributor

miss-islington commented Oct 8, 2019

Sorry @pablogsal, I had trouble checking out the 3.7 backport branch.
Please backport using cherry_picker on command line.
cherry_picker 10cd00a9e3c22af37c748ea5a417f6fb66601e21 3.7

@miss-islington
Copy link
Contributor

miss-islington commented Oct 8, 2019

Sorry, @pablogsal, I could not cleanly backport this to 3.6 due to a conflict.
Please backport using cherry_picker on command line.
cherry_picker 10cd00a9e3c22af37c748ea5a417f6fb66601e21 3.6

pablogsal added a commit to pablogsal/cpython that referenced this pull request Oct 8, 2019
@pablogsal
bpo-38395: Fix ownership in weakref.proxy methods (pythonGH-16632)
1b5f010 
The implementation of weakref.proxy's methods call back into the Python
API using a borrowed references of the weakly referenced object
(acquired via PyWeakref_GET_OBJECT). This API call may delete the last
reference to the object (either directly or via GC), leaving a dangling
pointer, which can be subsequently dereferenced.

To fix this, claim a temporary ownership of the referenced object when
calling the appropriate method. Some functions because at the moment they
do not need to access the borrowed referent, but to protect against
future changes to these functions, ownership need to be fixed in
all potentially affected methods.

(cherry picked from commit 10cd00a)
pablogsal added a commit to pablogsal/cpython that referenced this pull request Oct 8, 2019
@pablogsal
[3.8] bpo-38395: Fix ownership in weakref.proxy methods (pythonGH-16632)
2dd389f 
The implementation of weakref.proxy's methods call back into the Python
API using a borrowed references of the weakly referenced object
(acquired via PyWeakref_GET_OBJECT). This API call may delete the last
reference to the object (either directly or via GC), leaving a dangling
pointer, which can be subsequently dereferenced.

To fix this, claim a temporary ownership of the referenced object when
calling the appropriate method. Some functions because at the moment they
do not need to access the borrowed referent, but to protect against
future changes to these functions, ownership need to be fixed in
all potentially affected methods..
(cherry picked from commit 10cd00a)

Co-authored-by: Pablo Galindo <Pablogsal@gmail.com>
@python python deleted a comment from bedevere-bot Oct 8, 2019
pablogsal added a commit to pablogsal/cpython that referenced this pull request Oct 8, 2019
@pablogsal
[3.8] bpo-38395: Fix ownership in weakref.proxy methods (pythonGH-16632)
edb01a6 
The implementation of weakref.proxy's methods call back into the Python
API using a borrowed references of the weakly referenced object
(acquired via PyWeakref_GET_OBJECT). This API call may delete the last
reference to the object (either directly or via GC), leaving a dangling
pointer, which can be subsequently dereferenced.

To fix this, claim a temporary ownership of the referenced object when
calling the appropriate method. Some functions because at the moment they
do not need to access the borrowed referent, but to protect against
future changes to these functions, ownership need to be fixed in
all potentially affected methods..
(cherry picked from commit 10cd00a)

Co-authored-by: Pablo Galindo <Pablogsal@gmail.com>
@bedevere-bot
Copy link

bedevere-bot commented Oct 8, 2019

GH-16663 is a backport of this pull request to the 3.7 branch.

@vstinner
Copy link
Member

vstinner commented Oct 8, 2019

@pablogsal pablogsal removed the needs backport to 3.6 label 1 hour ago

I don't think that this change can be qualified as a fix for a security issue, and 3.6 no longer accept bugfixes. So yeah, no need to backport to 3.6 :-)

pablogsal added a commit to pablogsal/cpython that referenced this pull request Oct 8, 2019
@pablogsal
[3.7] bpo-38395: Fix ownership in weakref.proxy methods (pythonGH-16632)
b68f6d6 
The implementation of weakref.proxy's methods call back into the Python
API using a borrowed references of the weakly referenced object
(acquired via PyWeakref_GET_OBJECT). This API call may delete the last
reference to the object (either directly or via GC), leaving a dangling
pointer, which can be subsequently dereferenced.

To fix this, claim a temporary ownership of the referenced object when
calling the appropriate method. Some functions because at the moment they
do not need to access the borrowed referent, but to protect against
future changes to these functions, ownership need to be fixed in
all potentially affected methods..
(cherry picked from commit 10cd00a)

Co-authored-by: Pablo Galindo <Pablogsal@gmail.com>
pablogsal added a commit that referenced this pull request Oct 8, 2019
@pablogsal
[3.8] bpo-38395: Fix ownership in weakref.proxy methods (GH-16632) (G…
526ef85 
…H-16662)

The implementation of weakref.proxy's methods call back into the Python
API using a borrowed references of the weakly referenced object
(acquired via PyWeakref_GET_OBJECT). This API call may delete the last
reference to the object (either directly or via GC), leaving a dangling
pointer, which can be subsequently dereferenced.

To fix this, claim a temporary ownership of the referenced object when
calling the appropriate method. Some functions because at the moment they
do not need to access the borrowed referent, but to protect against
future changes to these functions, ownership need to be fixed in
all potentially affected methods..
(cherry picked from commit 10cd00a)

Co-authored-by: Pablo Galindo <Pablogsal@gmail.com>
miss-islington pushed a commit that referenced this pull request Oct 11, 2019
@pablogsal @miss-islington
[3.7] bpo-38395: Fix ownership in weakref.proxy methods (GH-16632) (G…
193366e 
…H-16663)

The implementation of weakref.proxy's methods call back into the Python
API using a borrowed references of the weakly referenced object
(acquired via PyWeakref_GET_OBJECT). This API call may delete the last
reference to the object (either directly or via GC), leaving a dangling
pointer, which can be subsequently dereferenced.

To fix this, claim a temporary ownership of the referenced object when
calling the appropriate method. Some functions because at the moment they
do not need to access the borrowed referent, but to protect against
future changes to these functions, ownership need to be fixed in
all potentially affected methods..
(cherry picked from commit 10cd00a)

Co-authored-by: Pablo Galindo <Pablogsal@gmail.com>





https://bugs.python.org/issue38395
jacobneiltaylor pushed a commit to jacobneiltaylor/cpython that referenced this pull request Dec 5, 2019
@pablogsal
bpo-38395: Fix ownership in weakref.proxy methods (pythonGH-16632)
51e598b 
The implementation of weakref.proxy's methods call back into the Python
API using a borrowed references of the weakly referenced object
(acquired via PyWeakref_GET_OBJECT). This API call may delete the last
reference to the object (either directly or via GC), leaving a dangling
pointer, which can be subsequently dereferenced.

To fix this, claim a temporary ownership of the referenced object when
calling the appropriate method. Some functions because at the moment they
do not need to access the borrowed referent, but to protect against
future changes to these functions, ownership need to be fixed in
all potentially affected methods.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@vstinner vstinner

@pitrou pitrou

@nascheme nascheme

Assignees

@pablogsal pablogsal

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
6 participants
@pablogsal @miss-islington @bedevere-bot @vstinner @pitrou @the-knights-who-say-ni