Returning None from a callback with restype py_object decrements None's refcount too much #81061
Comments
|
This occurs when writing a ctypes callback in Python whose restype is ctypes.py_object. If the callback returns None, the refcount of None is decremented once too often. This happens every time the callback is called, and if done often enough, Python attempts to deallocate None and crashes. To reproduce: $ bin/python3
Python 3.8.0a4+ (heads/master:09532feeec, May 10 2019, 23:53:49)
[Clang 8.0.0 (clang-800.0.42.1)] on darwin
Type "help", "copyright", "credits" or "license" for more information.
>>> import sys
>>> import ctypes
>>> FUNTYPE = ctypes.CFUNCTYPE(ctypes.py_object)
>>> @FUNTYPE
... def fun():
... return None
...
>>> print(fun())
None
>>> sys.getrefcount(None)
4329
>>> print(fun())
None
>>> sys.getrefcount(None)
4327
>>> print(fun())
None
>>> sys.getrefcount(None)
4326
>>> while True:
... fun()
...
Fatal Python error: deallocating None
Current thread 0x00007fff7bf80000 (most recent call first):
File "<stdin>", line 2 in <module>
Abort trap: 6
# exits with code 134 (SIGABRT)I've attached the crash report generated by OS X. (It's a plain text file, despite the extension.) Interestingly, this only happens with None. Other returned objects are refcounted correctly: >>> thing = object()
>>> @FUNTYPE
... def otherfun():
... return thing
...
>>> print(otherfun())
<object object at 0x10d920220>
>>> sys.getrefcount(thing)
2
>>> print(otherfun())
<object object at 0x10d920220>
>>> sys.getrefcount(thing)
2
>>> print(otherfun())
<object object at 0x10d920220>
>>> sys.getrefcount(thing)
2I could reproduce this on the following configurations:
|
dgelessus
mannequin
added
3.7 (EOL)
|
Thanks, I'll check this out |
|
The documentation says:
But that doesn't describe the situation you've shared. I'll continue to look into the ctypes module |
|
From lldb Current thread 0x00000001005c85c0 (most recent call first): object : <refcnt -1 at 0x10038a2c0>
|
|
Full trace for reference: (lldb) bt all
|
|
In the stack it's calling none_dealloc() which should never happen. Assume this is being triggered by ctypes PyCFuncPtr_call. The stacktrace I'm getting comes after the double decref so it's not showing the root cause. Someone who knows ctypes better might be able to help |
|
This is due to an oversight in _CallPythonObject in Modules/_ctypes/callbacks.c. The setfunc protocol is to return None if there's no object to keep alive. This isn't applicable to py_object (i.e. O_set in Modules/_ctypes/cfield.c). So the problem is the unconditional decref of None in the following snippet from _CallPythonObject: if (keep == NULL) /* Could not convert callback result. */
PyErr_WriteUnraisable(callable);
else if (keep == Py_None) /* Nothing to keep */
Py_DECREF(keep);
else if (setfunc != _ctypes_get_fielddesc("O")->setfunc) {
if (-1 == PyErr_WarnEx(PyExc_RuntimeWarning,
"memory leak in callback function.",
1))
PyErr_WriteUnraisable(callable);
}I'd rewrite it as follows: if (keep == NULL) { /* Could not convert callback result. */
PyErr_WriteUnraisable(callable);
} else if (setfunc != _ctypes_get_fielddesc("O")->setfunc) {
if (keep == Py_None) { /* Nothing to keep */
Py_DECREF(keep);
} else if (PyErr_WarnEx(PyExc_RuntimeWarning, "memory leak "
"in callback function.", 1) == -1) {
PyErr_WriteUnraisable(callable);
}
} |
|
Thanks Eryk that saved a lot of debugging. dgelessus - if you want to write a patch for CPython am happy to take you through this and get it over the line. Else: am Happy to write a test against the gc counter and a patch for this |
|
Thank you for looking into this! I can confirm that Eryk Sun's change fixes the issue for me locally. I'm up for making the patch for this. Regarding tests, I see there are already some refcount-related ctypes tests in Lib/ctypes/test/test_refcounts.py - should I add another test case there that reproduces this situation and ensures that None's refcount is unaffected? (I imagine testing against None's refcount will be a bit fragile, so it might be best to call the previously-buggy function in a loop and check afterwards that None's refcount hasn't changed significantly.) |
|
If you can write a test similar to the AnotherLeak.test_callback test case, and commit that first. It will show in the CI/CD log as failed and verify the issue (incase it comes up in PR review) Then add another commit with the patch itself and we should see the CI/CD pass. Please follow the PR guide if this is your first PR to CPython https://devguide.python.org/pullrequest/ |
eryksun
added
3.9
…es.py_object` callback (pythonGH-13364) (cherry picked from commit 837ba05) Co-authored-by: dgelessus <dgelessus@users.noreply.github.com>
…es.py_object` callback (pythonGH-13364) (cherry picked from commit 837ba05) Co-authored-by: dgelessus <dgelessus@users.noreply.github.com>
|
Thanks, looks like this was completed |
Nonefrom actypes.py_objectcallback #13364Note: these values reflect the state of the issue at the time it was migrated and might not reflect the current state.
Show more details
GitHub fields:
bugs.python.org fields:
Linked PRs
Nonefrom actypes.py_objectcallback (GH-13364) #100877Nonefrom actypes.py_objectcallback (GH-13364) #100878The text was updated successfully, but these errors were encountered: