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.

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

Merged
merged 1 commit into from
Feb 17, 2024

Conversation

serhiy-storchaka
Copy link
Member

@serhiy-storchaka serhiy-storchaka commented Feb 17, 2024

@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

@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
…ds (pythonGH-115619)

(cherry picked from commit 090dd21)

Co-authored-by: Serhiy Storchaka <[email protected]>
serhiy-storchaka added a commit to serhiy-storchaka/cpython that referenced this pull request Feb 17, 2024
…ds (pythonGH-115619)

(cherry picked from commit 090dd21)

Co-authored-by: Serhiy Storchaka <[email protected]>
@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 added a commit that referenced this pull request Feb 17, 2024
woodruffw pushed a commit to woodruffw-forks/cpython that referenced this pull request Mar 4, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants