Repository navigation
Type punning (and strict aliasing) issue in Py_CLEAR() and Py_SETREF() macros: Python --enable-pystats is miscompiled #99701
Description
Activity
c03e05c is the first bad commit.
Building with
--with-pydebugseems to fix the issue (so it's a bit of a Heisenbug, I guess).Reverting
Py_SETREFon its own is enough to fix this segfault.Oh, interesting! I can reproduce the crash with the command:
$ rm ~/pyprefix -rf; git clean -fdx; ./configure --enable-pystats --prefix ~/pyprefix CFLAGS="-O2" && make && make installBut I cannot reproduce the crash using
CFLAGS="-O0"norCFLAGS="-O1". The bug requires at leastgcc -O2optimization level, orgcc -O3.Then I can reproduce the crash with the command:
./python -E -m ensurepip --upgrade --root=/
The pystats build is very noisy, so I disable the debug output at Python exit using a local change:
diff --git a/Python/specialize.c b/Python/specialize.c index cd09b188b7..98ff406dbc 100644 --- a/Python/specialize.c +++ b/Python/specialize.c @@ -205,6 +205,7 @@ _Py_StatsClear(void) void _Py_PrintSpecializationStats(int to_file) { + return; if (_py_stats == NULL) { return; }
I can reproduce the crash at commit 0124b5d with the following patch:
diff --git a/Modules/itertoolsmodule.c b/Modules/itertoolsmodule.c index 381ec3b31d..caeba6a3ba 100644 --- a/Modules/itertoolsmodule.c +++ b/Modules/itertoolsmodule.c @@ -992,6 +992,14 @@ static PyTypeObject teedataobject_type = { }; +#define MY_SETREF(dst, src) \ + do { \ + PyObject **_tmp_dst_ptr = _Py_CAST(PyObject**, &(dst)); \ + PyObject *_tmp_dst = (*_tmp_dst_ptr); \ + *_tmp_dst_ptr = _PyObject_CAST(src); \ + Py_DECREF(_tmp_dst); \ + } while (0) + static PyObject * tee_next(teeobject *to) { @@ -1001,7 +1009,7 @@ tee_next(teeobject *to) link = teedataobject_jumplink(to->dataobj); if (link == NULL) return NULL; - Py_SETREF(to->dataobj, (teedataobject *)link); + MY_SETREF(to->dataobj, (teedataobject *)link); to->index = 0; } value = teedataobject_getitem(to->dataobj, to->index);
Oh. That bug is complicated. It looks like a type punning or strict aliasing bug.
With the bug, tee_next() is miscomplicated because of a type punning issue related to Py_SETREF().
With gcc -O2, functions called by tee_next() are inlined in tee_next(): teedataobject_jumplink(), Py_XINCREF(), Py_DECREF() from Py_SETREF(), teedataobject_newinternal(), Py_INCREF() at the end of teedataobject_newinternal().
The problem is around
to->dataobj:- At the function entry, the value is stored into a register:
mov r12,QWORD PTR [rdi+0x10] - Py_SETREF() changes
to->dataobj, it writes into the stack:mov QWORD PTR [rbp+0x10],rax--rbpis a copy of the originalrdivalue at the function entry - To call
teedataobject_getitem(to->dataobj, to->index), the old value ofto->dataobjis retrieved fromr12register... which is outdated: oops!
With the old Py_SETREF() implementation, after Py_SETREF(), the old register is no longer used, and the new value of
to->dataobjis retried:mov r12,QWORD PTR [rbp+0x10](here a newr12register is used).IMO this problem is related to type punning or strict aliasing, I'm not sure of the name of the problem.
- With the old Py_SETREF(), the write was done into type
teedataobject*:to->dataobjtype isteedataobject* - With the new y_SETREF(), the write is done into type
PyObject*. But the first register (r13) readto->dataobjas ateedataobject*which is a different type. The compiler gets more freedom in terms of optimizations: "oh, Py_SETREF() uses a different type, we can still use ourr13register, no need to invalidate it!".
The C language is hard :-(
That's the problem. I'm not sure what's the best way to solve it. A simple fix is to revert c03e05c: don't fix issue #98724. Another fix would be to avoid the type punning / strict aliasing issue by adding hints to the compiler that it must invalidate its register. Or maybe use the correct type in the macro. Or maybe a type different than
PyObject*(andPyObject**for_tmp_dst_ptr) can be used? Anyway, now I need to sleep ;-)cc @serge-sans-paille who loves C bugs ;-)
- At the function entry, the value is stored into a register:
I'm now curious if _Py_SET_OPCODE() could be affected by the same sickness?
#define _Py_SET_OPCODE(word, opcode) \ do { ((unsigned char *)&(word))[0] = (opcode); } while (0)
Some macros are using advanced casting, but to read so I understand that they cannot be affected.
#define DK_ENTRIES(dk) \ (assert((dk)->dk_kind == DICT_KEYS_GENERAL), \ (PyDictKeyEntry*)(&((int8_t*)((dk)->dk_indices))[(size_t)1 << (dk)->dk_log2_index_bytes])) #define DK_UNICODE_ENTRIES(dk) \ (assert((dk)->dk_kind != DICT_KEYS_GENERAL), \ (PyDictUnicodeEntry*)(&((int8_t*)((dk)->dk_indices))[(size_t)1 << (dk)->dk_log2_index_bytes]))
The Py_ARRAY_LENGTH() macro uses a hack using
typeof()to trigger a compiler error when the argument is a pointer, rather than an array./* Two gcc extensions. &a[0] degrades to a pointer: a different type from an array */ #define Py_ARRAY_LENGTH(array) \ (sizeof(array) / sizeof((array)[0]) \ + Py_BUILD_ASSERT_EXPR(!__builtin_types_compatible_p(typeof(array), \ typeof(&(array)[0]))))
- changed the title
[-]Segmentation fault running `ensurepip` during an `--enable-pystats` build[/-][+]Type punning (and strict aliasing) issue in Py_CLEAR() and Py_SETREF() macros: Python --enable-pystats is miscompiled[/+]on Dec 7, 2022
Bug report
When building with
--enable-pystats, theensurepipstep fails with a segmentation fault.The v3.11.0 tag works on this machine,
mainis currently broken. I have not yet bisected it.Cc @markshannon as the primary pystats author.
Your environment
Debian bookworm, WSL
Output during build
Backtrace from the segfault