8000 gh-115618: Remove improper Py_XDECREFs in property methods by serhiy-storchaka · Pull Request #115619 · python/cpython · GitHub
[go: up one dir, main page]
Skip to content

gh-115618: Remove improper Py_XDECREFs in property methods #115619

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
Merged
merged 1 commit into from
Feb 17, 2024
Merged

gh-115618: Remove improper Py_XDECREFs in property methods #115619

merged 1 commit into from
Feb 17, 2024

Conversation

serhiy-storchaka
Copy link
Member
@serhiy-storchaka serhiy-storchaka commented Feb 17, 2024 •
edited by bedevere-app bot
Loading

@serhiy-storchaka serhiy-storchaka added needs backport to 3.11 only security fixes needs backport to 3.12 only security fixes labels Feb 17, 2024
@bedevere-app bedevere-app bot mentioned this pull request Feb 17, 2024
Closed
@serhiy-storchaka
Copy link
Member Author

It has no effect in main, but in 3.11 the test crashes without the fix.

@eltoder
Copy link
Contributor
eltoder commented Feb 17, 2024

@serhiy-storchaka Currently your test passes on 3.12 and main without the fix, because None is immortal. AFAICT it is not possible to write a test to catch refcount bugs for immortal objects. What do you think if we change the debug versions of Py_INCREF/Py_DECREF to update the stats even for immortal objects? Then your test will catch this bug even on 3.12+.

@Eclips4
Copy link
Member
Eclips4 commented Feb 17, 2024 •
edited
Loading

@serhiy-storchaka Currently your test passes on 3.12 and main without the fix, because None is immortal. AFAICT it is not possible to write a test to catch refcount bugs for immortal objects. What do you think if we change the debug versions of Py_INCREF/Py_DECREF to update the stats even for immortal objects? Then your test will catch this bug even on 3.12+.

Personally I do not think that this is worth the effort, and this isn't will make test catch this bug (because this test goes along with the fix), it's just make bug reproducible.

Actually there's a test (Lib/test/test_capi/test_immortal) which relies on current behaviour.

@eltoder
Copy link
Contributor
eltoder commented Feb 17, 2024

@Eclips4 no, that test doesn't do anything with the total refcount, it only checks that decrefing an immortal object doesn't change its refcount. So it will be fine. We can improve the test by checking incref as well, which will also make it balanced. The test also hard-codes the boundaries of the smallint cache instead of using the macros, which can also be improved.

@serhiy-storchaka
Copy link
Member Author

What do you think if we change the debug versions of Py_INCREF/Py_DECREF to update the stats even for immortal objects?

I afraid that it is too late, and many tests will show unbalanced refcounts.

@serhiy-storchaka serhiy-storchaka merged commit 090dd21 into python:main Feb 17, 2024
@miss-islington-app
Copy link

Thanks @serhiy-storchaka for the PR 🌮🎉.. I'm working now to backport this PR to: 3.11, 3.12.
🐍🍒⛏🤖

@serhiy-storchaka serhiy-storchaka deleted the property-decref-none branch February 17, 2024 21:18
@miss-islington-app
Copy link

Sorry, @serhiy-storchaka, I could not cleanly backport this to 3.12 due to a conflict.
Please backport using cherry_picker on command line.

cherry_picker 090dd21ab9379d6a2a6923d6cbab697355fb7165 3.12

@miss-islington-app
Copy link

Sorry, @serhiy-storchaka, I could not cleanly backport this to 3.11 due to a conflict.
Please backport using cherry_picker on command line.

cherry_picker 090dd21ab9379d6a2a6923d6cbab697355fb7165 3.11

serhiy-storchaka added a commit to serhiy-storchaka/cpython that referenced this pull request Feb 17, 2024
@serhiy-storchaka
[3.11] pythongh-115618: Remove improper Py_XDECREFs in property metho…
890c3ac 
…ds (pythonGH-115619)

(cherry picked from commit 090dd21)

Co-authored-by: Serhiy Storchaka <storchaka@gmail.com>
serhiy-storchaka added a commit to serhiy-storchaka/cpython that referenced this pull request Feb 17, 2024
@serhiy-storchaka
[3.12] pythongh-115618: Remove improper Py_XDECREFs in property metho…
98fd479 
…ds (pythonGH-115619)

(cherry picked from commit 090dd21)

Co-authored-by: Serhiy Storchaka <storchaka@gmail.com>
@bedevere-app
Copy link
bedevere-app bot commented Feb 17, 2024

GH-115620 is a backport of this pull request to the 3.12 branch.

@bedevere-app bedevere-app bot removed the needs backport to 3.12 only security fixes label Feb 17, 2024
@bedevere-app
Copy link
bedevere-app bot commented Feb 17, 2024

GH-115621 is a backport of this pull request to the 3.11 branch.

@bedevere-app bedevere-app bot removed the needs backport to 3.11 only security fixes label Feb 17, 2024
serhiy-storchaka added a commit that referenced this pull request Feb 17, 2024
@serhiy-storchaka
[3.12] gh-115618: Remove improper Py_XDECREFs in property methods (GH…
b9d1efa 
…-115619) (GH-115620)

(cherry picked from commit 090dd21)
serhiy-storchaka added a commit that referenced this pull request Feb 17, 2024
@serhiy-storchaka
[3.11] gh-115618: Remove improper Py_XDECREFs in property methods (GH…
5dac9c5 
…-115619) (GH-115621)

(cherry picked from commit 090dd21)
woodruffw pushed a commit to woodruffw-forks/cpython that referenced this pull request Mar 4, 2024
@serhiy-storchaka @woodruffw
pythongh-115618: Remove improper Py_XDECREFs in property methods (pyt…
f1fa7cb 
…honGH-115619)
diegorusso pushed a commit to diegorusso/cpython that referenced this pull request Apr 17, 2024
@serhiy-storchaka @diegorusso
pythongh-115618: Remove improper Py_XDECREFs in property methods (pyt…
234cee4 
…honGH-115619)
LukasWoodtli pushed a commit to LukasWoodtli/cpython that referenced this pull request Jan 22, 2025
@serhiy-storchaka @LukasWoodtli
pythongh-115618: Remove improper Py_XDECREFs in property methods (pyt…
563be6a 
…honGH-115619)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Eclips4 Eclips4 Eclips4 approved these changes

Assignees

@serhiy-storchaka serhiy-storchaka

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@serhiy-storchaka @eltoder @Eclips4
0