Repository navigation
Incomplete tuple created by PyTuple_New() and accessed via the GC can trigged a crash #59313
Description
Activity
pxd commented
on Jun 19, 2012 pxdmannequinMannequinAuthorHi,
Sporadically, while running an sqlite3 query, the above error is seen.
In order to debug, I modified Objects/tupleobject.c, PyTuple_SetItem() as follows:if (!PyTuple_Check(op) || op->ob_refcnt != 1) { Py_XDECREF(newitem); /* * Temp: Bug XYZ Generate core so that we can debug * * PyErr_BadInternalCall(); * return -1; */ char errmsg[200]; sprintf(errmsg, "Bug XYZ: PyTuple_Check(op) = %d " "op->ob_refcnt = %d; see core\n", PyTuple_Check(op), op->ob_refcnt); Py_FatalError(errmsg); }
This generates a core with the following bt. Showing the top few frames only:
(gdb) bt
#0 0x0000000800acd3fc in thr_kill () at thr_kill.S:3
#1 0x0000000800b5e283 in abort () at /build/mnt/src/lib/libc/stdlib/abort.c:65
#2 0x0000000000494acf in Py_FatalError (msg=Variable "msg" is not available.
) at ./../Python/pythonrun.c:1646
#3 0x000000000044e740 in PyTuple_SetItem (op=0x80c6e6308, i=16, newitem=0x80a80d780) at ./../Objects/tupleobject.c:128
#4 0x0000000807298866 in _pysqlite_fetch_one_row (self=0x80b846e48) at _sqlite/cursor.c:402
#5 0x0000000807298bf5 in pysqlite_cursor_iternext (self=0x80b846e48) at _sqlite/cursor.c:898
#6 0x0000000000476943 in PyEval_EvalFrameEx (f=0x80a94d420, throwflag=Variable "throwflag" is not available.
) at ./../Python/ceval.c:2237
#7 0x000000000047acbf in PyEval_EvalFrameEx (f=0x80a94b820, throwflag=Variable "throwflag" is not available.
) at ./../Python/ceval.c:3765
#8 0x000000000047bf09 in PyEval_EvalCodeEx (co=0x808d2ac60, globals=Variable "globals" is not available.
) at ./../Python/ceval.c:2942
...
(gdb) fr 4
#4 0x0000000807298866 in _pysqlite_fetch_one_row (self=0x80b846e48) at _sqlite/cursor.c:402
402 PyTuple_SetItem(row, i, converted);
Current language: auto; currently c
(gdb) l
397 converted = buffer;
398 }(gdb) p *(PyTupleObject *)row
$11 = {ob_refcnt = 2, ob_type = 0x60fee0, ob_size = 22, ob_item = {0x80a534030}}'row' was allocated via PyTuple_New()
but, somehow its refcount has become 2 while setting the 16th item!!!Is this a known issue? If not, what can I do to debug.
Thanks,
PankajCould you possibly reproduce this in 2.7, 3.2 and/or "default" (future 3.3)?.
Python 2.6 support is over.
pxd commented
on Jun 19, 2012 pxdmannequinMannequinAuthorsorry, 2.7, 3.2 is not an option currently but I am hoping someone can
provide enough info to help probe this more efficiently. There seem to be references to this issue on the web but no root-cause.pxd commented
on Jun 28, 2012 pxdmannequinMannequinAuthorI believe I have found the root-cause for this issue.
It is occurring due to the use of the garbage collector in another “memMonitor” thread (we run it periodically to get stats on objects, track mem leaks, etc). Since _pysqlite_fetch_one_row() releases the GIL before calling PyTuple_SetItem(), if the memMonitor is scheduled to run and, say, calls gc.get_objects(), it increments the refcount on all tracked objects (via append_objects()->PyList_Append()->app1()->PY_INCREF()). I have stack traces to confirm. This seems to rule out the use of gc methods (such as get_objects(), get_referrers/referents()) in multi-threaded programs or have them handle SystemError arising from such usage. Agree?
Thanks for the analysis!
This is quite similar to bpo-793822: gc.get_referrers() can access unfinished tuples. The difference here is that what breaks is not the monitoring tool, but the "main" program!Here is a simple script inspired from the original bug; PySequence_Tuple() uses PyTuple_SET_ITEM() which is a macro without the ob_refcnt check, but we can make it call _PyTuple_Resize() and fail there. All versions of CPython are affected:
import gc TAG = object() def monitor(): lst = [x for x in gc.get_referrers(TAG) if isinstance(x, tuple)] t = lst[0] # this *is* the result tuple print(t) # full of nulls ! return t # Keep it alive for some time def my_iter(): yield TAG # 'tag' gets stored in the result tuple t = monitor() for x in range(10): yield x # SystemError when the tuple needs to be resized tuple(my_iter())
I wonder why _pysqlite_fetch_one_row() releases the GIL around sqlite3_column_type(). By its name, it doesn't sound like an expensive function.
Another workaround would be to call PyTuple_SET_ITEM instead of PyTuple_SetItem.
pxd commented
on Jun 28, 2012 pxdmannequinMannequinAuthorWondering the same thing myself, and yes sqlite3_column_type() by itself doesn't seem expensive. I assumed in general it was to allow more responsiveness for apps with huge number of columns (i.e. large tuple size). But we have about 20-25 columns and so I was going to try removing it and seeing the
results. In any case, it seems, fewer GIL acquire/releases will help with throughput.
Are there any guidelines on when GIL should be released?Re PyTuple_SET_ITEM...yes that's also a possibility but it would then hide genuine bugs.
Are there any guidelines on when GIL should be released?
The GIL should be released:
- for CPU-heavy external functions (e.g. compression, cryptography)
- for external functions which wait for I/O
Re PyTuple_SET_ITEM...yes that's also a possibility but it would then
hide genuine bugs.Well, as long as your monitor only increfs the tuple and doesn't mutate
it, there shouldn't be any problem. We use PyTuple_SET_ITEM in many
other places.lkraav commented
on Oct 3, 2014 lkraavmannequinMannequinI may be seeing this running tracd-1.1.2b1 on python 2.7.7
What is tracd doing? This issue should only appear when calling one of the debugging functions in the gc module, AFAICT.
Still happening in 3.10:
Python 3.10.0a5+ (heads/master:bf2e7e55d7, Feb 11 2021, 23:09:25) [MSC v.1928 64 bit (AMD64)] on win32 Type "help", "copyright", "credits" or "license" for more information. >>> import gc >>> TAG = object() >>> >>> def monitor(): ... lst = [x for x in gc.get_referrers(TAG) ... if isinstance(x, tuple)] ... t = lst[0] # this *is* the result tuple ... print(t) # full of nulls ! ... return t # Keep it alive for some time ... >>> def my_iter(): ... yield TAG # 'tag' gets stored in the result tuple ... t = monitor() ... for x in range(10): ... yield x # SystemError when the tuple needs to be resized ... >>> tuple(my_iter()) (<object object at 0x00000217225091B0>, <NULL>, <NULL>, <NULL>, <NULL>, <NULL>, <NULL>, <NULL>, <NULL>, <NULL>) Traceback (most recent call last): File "<stdin>", line 1, in <module> SystemError: C:\Users\User\src\cpython-dev\Objects\tupleobject.c:963: bad argument to internal function >>>
5 remaining items
IMO the unsafe PyTuple_New() API should be avoided.
Is not that simple, there are other APIs that track the tuple as _PyTuple_Resize. This problem also is not unique to tuples, although is mainly prominent in them.
We have this warning in the docs:
> Care must be taken when using objects returned by get_referrers() because some of them could still be under construction and hence in a temporarily invalid state. Avoid using get_referrers() for any purpose other than debugging.
That's because *by thesign* these APIs can potentially access half-initialized objects.
I don't know if is worth to add a new API just for tuples, given that this problem happens with many other objects
There are other safe alternatives like Py_BuildValue("(OOO)", item1, item2, item3).
That's a lot slower unfortunately
If the GIL is released before the tuple is fully populated and something access to this tuple via the GC (ex: gc.get_objects()), accessing the tuple can crash, especially in the Python land (for example, repr(the_tuple) is likely to crash).
It can happen even without releasing the GIL: A new tuple is created, then some other object is created using the CAPI, the gc runs, the callback triggers (or the tuplevisit method is invoked) and then kaboom
That's a lot slower unfortunately
Ah sorry, I forgot PyTuple_Pack(3, item1, item2, item3) which should be very efficient. This function is also safe: only track the tuple when it is fully initialized.
This problem also is not unique to tuples, although is mainly prominent in them.
PyList_New() is also affected. Do you think about other types?
PyDict_New() and PySet_New() create empty containers and so are ok.
PyList_New() is also affected. Do you think about other types?
Any C extension class that implements a new_whatever() method that leaves the class tracked and not ready.
Any C extension class that implements a new_whatever() method that leaves the class tracked and not ready.
I'm not aware of such C extension but they likely exists. If we have such extensions in the stdlib, we can try to fix them.
We cannot fix such GC bugs in third party code, and I don't think that we can hack the GC to work around this issue neither.
Should we still fix sqlite3, or wait for an agreement on python/issues-test-cpython#24510?
Should we still fix sqlite3, or wait for an agreement on python/issues-test-cpython#24510?
I suggest to let's all agree on how to fix this on the bigger scale first.
Unless there is a simple, reliable, cheap, and universal solution at hand, consider closing this. Given how long PyTuple_New() has exist, it doesn't seem to be much of a problem in the real world.
Historically, we punted on "crashers" involving either gc.get_referrers or byte code hacks. Both of those reach inside Python's black box, allowing disruption of otherwise sensible invariants.
I mainly agree on closing this issue as we already have a warning about this behaviour in gc.get_referrers and other friends.
On the other hand I am still a bit afraid of a crash that could happen if the GC does a pass and one of these tuples is in an inconsistent state. Is possible that this crashes Python without the user calling ever any function from the GC module. I need to do some investigation around this, but I agree that this is a separate issue, so I will open a new one if it happens to be relevant.
I'm pretty sure the problem is not only with
gc.get_referrers(). The GC expects that objects that are tracked are valid, at least in the sense that tp_tranverse and tp_clear can be called on them. I suppose if they have finalizers, then the finalizers could run too.Any C extension class that implements a new_whatever() method that leaves the class tracked and not ready.
That sounds like a crash waiting to happen to me. You can get away with it if the GC happens to not run until the class becomes ready.
Note: 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