GH-81061: Fix refcount issue when returning `None` from a `ctypes.py_object` callback by dgelessus · Pull Request #13364 · python/cpython · GitHub

Right now this only adds a failing test for the issue, to check that the bug is properly detected in CI. Once that runs through I'll push the commit with the actual fix. Done

https://bugs.python.org/issue36880

Issue: Returning None from a callback with restype py_object decrements None's refcount too much #81061

Thank you to Eryk Sun for finding the cause of the bug and providing the code fix.

Previously, our ctypes_patch would trigger a bug in ctypes (see https://bugs.python.org/issue36880 and python/cpython#13364), which would eventually crash Python if done too often. This workaround changes our ctypes_patch implementation so that it no longer triggers the ctypes bug. This fixes the crash reported in beeware/toga#549.

I don't feel competent to review ctypes issues, sorry. Perhaps @serhiy-storchaka

There are merge conflicts.

A Python core developer has requested some changes be made to your pull request before we can consider merging it. If you could please address their requests along with any other requests in other reviews from core developers that would be appreciated.

Once you have made the requested changes, please leave a comment on this pull request containing the phrase I have made the requested changes; please review again. I will then notify any core developers who have left a review that you're ready for them to take another look at this pull request.

# Conflicts: # Modules/_ctypes/callbacks.c

Fixed the conflict, thanks. I have made the requested changes; please review again.

Thanks for making the requested changes!

@isidentical, @kumaraditya303: please review the changes made to this pull request.

AFAICT, this test is rendered pointless in 3.12 due to the adoption of PEP 683 (Immortal Objects, Using a Fixed Refcount). Because None is immortal, executing Py_INCREF(Py_None) and Py_DECREF(Py_None) will no longer modify its refcount.

Ah, good to know. I assume this isn't implemented yet though, because as of the current state of main (447d061), this test still hard-crashes without the fix.

Yeah, in general we don't do these kind of tests. We have an automatic refleak detection mechanism but it will only work if the path that leaks references is exercised in the test suite. This means that you can add a test that ensures that the code you are adding to callbacks.c is actually executed but your test doesn't need to try to detect the refleak, that will be done automatically. I suppose the reason this was never detected is that we don't have a test that actually runs that.

Ah okay. I was going by what the other tests in test_refcounts are doing. Should I adjust those as well to not check sys.getrefcount manually?

Should I adjust those as well to not check sys.getrefcount manually?

Not as part of this issue (one issue per problem - only changing what's required for the problem at hand). I haven't checked those and maybe these are checked more specific things.

You can confirm that your test work running `./python -m test test_ctypes -R'.

Ensure that it detects the problem without your fix first :)

Thanks, I didn't know that just -m test works too (I used -m test.regrtest). Yes, I checked - with -R it now indeed detects the original problem.

Co-authored-by: Eryk Sun <eryksun@gmail.com>

Single call should be enough to trigger the refleak checker.

Unfortunately not... I tested again - apparently the refleak checker (with default settings: -m test --huntrleaks=: test.test_ctypes.test_refcounts) only detects if None's refcount is too high, not if it's too low. The latter case is only detected once None's refcount gets to zero/negative (which triggers an assertion), and that requires a thousand or so iterations of the broken code.

If None refcount goes too small it will crash at some point assuming the rest of the code decrements normally

Unfortunately not... I tested again - apparently the refleak checker (with default settings: -m test --huntrleaks=: test.test_ctypes.test_refcounts) only detects if None's refcount is too high, not if it's too low. The latter case is only detected once None's refcount gets to zero/negative (which triggers an assertion), and that requires a thousand or so iterations of the broken code.

I see, it is because of #74959.

🤖 New build scheduled with the buildbot fleet by @kumaraditya303 for commit 66ea175 🤖

If you want to schedule another build, you need to add the :hammer: test-with-refleak-buildbots label again.

Buildbots are happy, merging. Thanks for the PR.

Thanks @dgelessus for the PR, and @kumaraditya303 for merging it 🌮🎉.. I'm working now to backport this PR to: 3.10, 3.11.
🐍🍒⛏🤖

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

GH-100878 is a backport of this pull request to the 3.10 branch.

…es.py_object` callback (pythonGH-13364) (cherry picked from commit 837ba05) Co-authored-by: dgelessus <dgelessus@users.noreply.github.com>

…object` callback (GH-13364) (cherry picked from commit 837ba05) Co-authored-by: dgelessus <dgelessus@users.noreply.github.com>

bpo-36880: Add failing test, fix to follow shortly

85026c5

the-knights-who-say-ni added the CLA signed label May 16, 2019

bedevere-bot added tests Tests in the Lib/test dir awaiting review labels May 16, 2019

bpo-36880: Fix refcount issue with ctypes callbacks returning None

2a5aeba

Thank you to Eryk Sun for finding the cause of the bug and providing the code fix.

dgelessus marked this pull request as ready for review May 16, 2019 17:46

isidentical approved these changes May 16, 2019

View reviewed changes

bedevere-bot added awaiting core review and removed awaiting review labels May 16, 2019

dgelessus mentioned this pull request May 24, 2019

Add workaround for bpo-36880 crash to ctypes_patch beeware/rubicon-objc#136

Merged

csabella requested review from abalkin and pitrou May 28, 2019 20:52

pablogsal requested a review from serhiy-storchaka December 19, 2019 15:58

pablogsal assigned pablogsal and unassigned pablogsal Dec 19, 2019

ezio-melotti removed the CLA signed label Jul 13, 2022

kumaraditya303 requested changes Jan 1, 2023

View reviewed changes

bedevere-bot removed the awaiting core review label Jan 1, 2023

bedevere-bot added the awaiting changes label Jan 1, 2023

Merge branch 'main' into fix_ctypes_callback_none_refcount

611f1b6

# Conflicts: # Modules/_ctypes/callbacks.c

bedevere-bot added awaiting change review and removed awaiting changes labels Jan 1, 2023

bedevere-bot requested review from isidentical and kumaraditya303 January 1, 2023 21:49

kumaraditya303 removed the request for review from abalkin January 2, 2023 11:01

eryksun reviewed Jan 2, 2023

View reviewed changes

Comment thread Lib/test/test_ctypes/test_refcounts.py Outdated

eryksun reviewed Jan 2, 2023

View reviewed changes

pablogsal reviewed Jan 3, 2023

View reviewed changes

dgelessus and others added 2 commits January 3, 2023 13:15

Apply suggestions from code review

fb17c68

Co-authored-by: Eryk Sun <eryksun@gmail.com>

Remove manual sys.getrefcount check in favor of regrtest --huntrleaks

66ea175

kumaraditya303 reviewed Jan 3, 2023

View reviewed changes

kumaraditya303 added type-bug An unexpected behavior, bug, or error 🔨 test-with-refleak-buildbots Test PR w/ refleak buildbots; report in status section labels Jan 8, 2023

bedevere-bot removed the 🔨 test-with-refleak-buildbots Test PR w/ refleak buildbots; report in status section label Jan 8, 2023

kumaraditya303 approved these changes Jan 9, 2023

View reviewed changes

bedevere-bot added awaiting merge and removed awaiting change review labels Jan 9, 2023

kumaraditya303 changed the title bpo-36880: Fix refcount issue when returning None from a ctypes.py_object callback GH-81061: Fix refcount issue when returning None from a ctypes.py_object callback Jan 9, 2023

kumaraditya303 added needs backport to 3.10 only security fixes needs backport to 3.11 only security fixes labels Jan 9, 2023

kumaraditya303 merged commit 837ba05 into python:main Jan 9, 2023

bedevere-bot removed the awaiting merge label Jan 9, 2023

dgelessus mannequin mentioned this pull request Apr 10, 2022

Returning None from a callback with restype py_object decrements None's refcount too much #81061

Closed

bedevere-bot removed the needs backport to 3.11 only security fixes label Jan 9, 2023

bedevere-bot removed the needs backport to 3.10 only security fixes label Jan 9, 2023

miss-islington added a commit that referenced this pull request Jan 9, 2023

GH-81061 : Fix refcount issue when returning None from a `ctypes.py_…

b374481

…object` callback (GH-13364) (cherry picked from commit 837ba05) Co-authored-by: dgelessus <dgelessus@users.noreply.github.com>

miss-islington added a commit that referenced this pull request Jan 9, 2023

GH-81061 : Fix refcount issue when returning None from a `ctypes.py_…

2d1128e

…object` callback (GH-13364) (cherry picked from commit 837ba05) Co-authored-by: dgelessus <dgelessus@users.noreply.github.com>

freakboy3742 mentioned this pull request Sep 25, 2024

Remove workaround for python/cpython#81061. beeware/rubicon-objc#517

Merged

4 tasks

+                              # Allow for small variations in None's refcount from other
+                              # sources.
+                              self.assertAlmostEqual(
+                                  sys.getrefcount(None), none_refcount, delta=50)

		@support.refcount_test
		def test_callback_py_object_none_return(self):

+                                  return None
+                              # Check that calling func does not affect None's refcount.
+                              for _ in range(10000):

GH-81061: Fix refcount issue when returning `None` from a `ctypes.py_object` callback by dgelessus · Pull Request #13364 · python/cpython · GitHub

Uh oh!

Uh oh!

GH-81061: Fix refcount issue when returning None from a ctypes.py_object callback - #13364

GH-81061: Fix refcount issue when returning None from a ctypes.py_object callback#13364kumaraditya303 merged 5 commits intopython:mainpython/cpython:mainfrom dgelessus:fix_ctypes_callback_none_refcountdgelessus/cpython:fix_ctypes_callback_none_refcountCopy head branch name to clipboard

Conversation

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

Uh oh!

Uh oh!

pitrou commented May 29, 2019

Uh oh!

kumaraditya303 left a comment

Choose a reason for hiding this comment

Uh oh!

bedevere-bot commented Jan 1, 2023

Uh oh!

dgelessus commented Jan 1, 2023

Uh oh!

bedevere-bot commented Jan 1, 2023

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

pablogsal Jan 3, 2023 • edited by AlexWaygood Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

bedevere-bot commented Jan 8, 2023

Uh oh!

kumaraditya303 commented Jan 9, 2023

Uh oh!

miss-islington commented Jan 9, 2023

Uh oh!

bedevere-bot commented Jan 9, 2023

Uh oh!

bedevere-bot commented Jan 9, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

10 participants

GH-81061: Fix refcount issue when returning None from a ctypes.py_object callback#13364
kumaraditya303 merged 5 commits into
python:mainfrom
dgelessus:fix_ctypes_callback_none_refcount

dgelessus commented May 16, 2019 •
edited by bedevere-bot
Loading

pablogsal Jan 3, 2023 •
edited by AlexWaygood
Loading