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

dgelessus · 2019-05-16T17:07:57Z

~~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.

pitrou · 2019-05-29T20:20:01Z

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

kumaraditya303

There are merge conflicts.

bedevere-bot · 2023-01-01T15:29:26Z

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

dgelessus · 2023-01-01T21:48:55Z

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

bedevere-bot · 2023-01-01T21:48:59Z

Thanks for making the requested changes!

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

Lib/test/test_ctypes/test_refcounts.py

eryksun · 2023-01-02T19:27:15Z

Lib/test/test_ctypes/test_refcounts.py

+    @support.refcount_test
+    def test_callback_py_object_none_return(self):


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.

pablogsal · 2023-01-03T11:56:14Z

Lib/test/test_ctypes/test_refcounts.py

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


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

kumaraditya303 · 2023-01-03T14:59:51Z

Lib/test/test_ctypes/test_refcounts.py

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


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.

bedevere-bot · 2023-01-08T14:11:09Z

🤖 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.

kumaraditya303 · 2023-01-09T10:11:37Z

Buildbots are happy, merging. Thanks for the PR.

miss-islington · 2023-01-09T10:13:08Z

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

bedevere-bot · 2023-01-09T10:13:24Z

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

bedevere-bot · 2023-01-09T10:13:28Z

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

…object` callback (GH-13364) (cherry picked from commit 837ba05) Co-authored-by: dgelessus <[email protected]>

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 pitrou and abalkin 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

Lib/test/test_ctypes/test_refcounts.py Outdated Show resolved Hide resolved

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

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

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

freakboy3742 mentioned this pull request Sep 25, 2024

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

Merged

4 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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 #13364

dgelessus commented May 16, 2019 •

edited by bedevere-bot

Loading

pitrou commented May 29, 2019

kumaraditya303 left a comment

bedevere-bot commented Jan 1, 2023

dgelessus commented Jan 1, 2023

bedevere-bot commented Jan 1, 2023

eryksun Jan 2, 2023

dgelessus Jan 3, 2023

pablogsal Jan 3, 2023

dgelessus Jan 3, 2023

pablogsal Jan 3, 2023 •

edited by AlexWaygood

Loading

pablogsal Jan 3, 2023

dgelessus Jan 3, 2023

kumaraditya303 Jan 3, 2023

dgelessus Jan 3, 2023

pablogsal Jan 3, 2023

kumaraditya303 Jan 8, 2023

bedevere-bot commented Jan 8, 2023

kumaraditya303 commented Jan 9, 2023

miss-islington commented Jan 9, 2023

bedevere-bot commented Jan 9, 2023

bedevere-bot commented Jan 9, 2023

		@support.refcount_test
		def test_callback_py_object_none_return(self):

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 #13364

Conversation

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

pitrou commented May 29, 2019

kumaraditya303 left a comment

Choose a reason for hiding this comment

bedevere-bot commented Jan 1, 2023

dgelessus commented Jan 1, 2023

bedevere-bot commented Jan 1, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pablogsal Jan 3, 2023 • edited by AlexWaygood Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bedevere-bot commented Jan 8, 2023

kumaraditya303 commented Jan 9, 2023

miss-islington commented Jan 9, 2023

bedevere-bot commented Jan 9, 2023

bedevere-bot commented Jan 9, 2023

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 #13364

dgelessus commented May 16, 2019 •

edited by bedevere-bot

Loading

pablogsal Jan 3, 2023 •

edited by AlexWaygood

Loading