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

pablogsal · 2019-10-08T00:23:42Z

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

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.

pitrou

LGTM, thank you

vstinner

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

Lib/test/test_weakref.py

pablogsal · 2019-10-08T11:10:28Z

@vstinner Done, can you review again?

vstinner

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

…macros

vstinner

LGTM. Goodbye borrowed references!

miss-islington · 2019-10-08T15:30:53Z

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

miss-islington · 2019-10-08T15:31:00Z

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 · 2019-10-08T15:31:02Z

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 · 2019-10-08T15:31:07Z

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

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)

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

bedevere-bot · 2019-10-08T15:57:34Z

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

vstinner · 2019-10-08T16:59:49Z

@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 :-)

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

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

…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 <[email protected]> https://bugs.python.org/issue38395

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.

the-knights-who-say-ni added the CLA signed label Oct 8, 2019

bedevere-bot added the awaiting core review label Oct 8, 2019

pablogsal requested a review from vstinner October 8, 2019 00:23

pablogsal added needs backport to 3.6 and removed needs backport to 2.7 labels Oct 8, 2019

pablogsal requested a review from nascheme October 8, 2019 00:26

pablogsal self-assigned this Oct 8, 2019

pablogsal removed the needs backport to 3.6 label Oct 8, 2019

pablogsal requested a review from pitrou October 8, 2019 00:31

pablogsal added needs backport to 3.6 labels Oct 8, 2019

pitrou approved these changes Oct 8, 2019

View reviewed changes

bedevere-bot added awaiting merge and removed awaiting core review labels Oct 8, 2019

vstinner reviewed Oct 8, 2019

View reviewed changes

Objects/weakrefobject.c Show resolved Hide resolved

Lib/test/test_weakref.py Show resolved Hide resolved

pablogsal added 2 commits October 8, 2019 12:01

Correct more functions

7f4fcc0

Add comment to the test

52cda14

pablogsal force-pushed the bpo-38395 branch from 54841a7 to 52cda14 Compare October 8, 2019 11:06

Fix weakref_repr

2fd33a3

Fix incorrect incref

8427579

pablogsal force-pushed the bpo-38395 branch from 7178c7e to 8427579 Compare October 8, 2019 13:15

vstinner reviewed Oct 8, 2019

View reviewed changes

Objects/weakrefobject.c Outdated Show resolved Hide resolved

Objects/weakrefobject.c Outdated Show resolved Hide resolved

Handle WRAP_UNARY(), WRAP_BINARY(), WRAP_TERNARY() and WRAP_METHOD() …

d44d9e3

…macros

vstinner approved these changes Oct 8, 2019

View reviewed changes

pablogsal removed the needs backport to 2.7 label Oct 8, 2019

pablogsal merged commit 10cd00a into python:master Oct 8, 2019

pablogsal deleted the bpo-38395 branch October 8, 2019 15:30

bedevere-bot removed the awaiting merge label Oct 8, 2019

bedevere-bot removed the needs backport to 3.8 label Oct 8, 2019

python deleted a comment from bedevere-bot Oct 8, 2019

bedevere-bot removed the needs backport to 3.7 label Oct 8, 2019

pablogsal removed the needs backport to 3.6 label Oct 8, 2019

terryjreedy mentioned this pull request Jul 2, 2023

startup-failure in the python idle #106333

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Uh oh!

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

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

Uh oh!

pablogsal commented Oct 8, 2019 •

edited by bedevere-bot

Loading

Uh oh!

pitrou left a comment

Uh oh!

vstinner left a comment

Uh oh!

Uh oh!

Uh oh!

pablogsal commented Oct 8, 2019

Uh oh!

vstinner left a comment

Uh oh!

Uh oh!

Uh oh!

vstinner left a comment

Uh oh!

miss-islington commented Oct 8, 2019

Uh oh!

miss-islington commented Oct 8, 2019

Uh oh!

miss-islington commented Oct 8, 2019

Uh oh!

miss-islington commented Oct 8, 2019

Uh oh!

bedevere-bot commented Oct 8, 2019

Uh oh!

vstinner commented Oct 8, 2019

Uh oh!

Uh oh!

Uh oh!

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

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

Uh oh!

Conversation

pablogsal commented Oct 8, 2019 • edited by bedevere-bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

pitrou left a comment

Choose a reason for hiding this comment

Uh oh!

vstinner left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

pablogsal commented Oct 8, 2019

Uh oh!

vstinner left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

vstinner left a comment

Choose a reason for hiding this comment

Uh oh!

miss-islington commented Oct 8, 2019

Uh oh!

miss-islington commented Oct 8, 2019

Uh oh!

miss-islington commented Oct 8, 2019

Uh oh!

miss-islington commented Oct 8, 2019

Uh oh!

bedevere-bot commented Oct 8, 2019

Uh oh!

vstinner commented Oct 8, 2019

Uh oh!

Uh oh!

pablogsal commented Oct 8, 2019 •

edited by bedevere-bot

Loading