Repository navigation
Macro Py_CLEAR references argument two times. #98724
Description
Activity
Here's an example of an expression in
PyClear:cpython/Modules/selectmodule.c
Lines 108 to 116 in 365852a
We also have:
static int template_clear(TemplateObject *self) { Py_CLEAR(self->literal); for (Py_ssize_t i = 0, n = Py_SIZE(self); i < n; i++) { Py_CLEAR(self->items[i].literal); } return 0; }
And
for (Py_ssize_t i = 0; i < keys->dk_nentries; i++) { Py_CLEAR(values->values[i]); }
All cases are element access, but this is the most complex examples I am able to find.
I am not sure that I made the point totally clear. Here is an example program for clarification, using some dummy type for
PyObject.#include <stdlib.h> #define _PyObject_CAST(op) ((PyObject*)(op)) #define Py_CLEAR(op) \ do { \ PyObject *_py_tmp = _PyObject_CAST(op); \ if (_py_tmp != NULL) { \ (op) = NULL; \ /* Py_DECREF(_py_tmp); */ \ free((void*)_py_tmp); \ } \ } while (0) typedef struct { } PyObject; int main() { PyObject* obj[16]; PyObject** p = obj; size_t i; for (i = 0; i < 16; i++) { obj[i] = malloc(sizeof(PyObject)); } #if 1 for (int i = 0; i < 16; i++) { Py_CLEAR(*p++); } #else for (int i = 0; i < 16; i++, p++) { Py_CLEAR(*p); } #endif }This code will give a buffer overflow. Changing the last line to
for (int i = 0; i < 16; i++, p++) Py_CLEAR(*p);works without problems. The side effect is not triggered by Python library code and most probably not in any sane C project, but it is not unconceivable that this macro shoots someone out there into the foot. I suggest that, if you have the choice, to somehow make all macros evaluate their arguments just once.I'm not sure we've ever promised that macros won't evaluate their args more than once. @vstinner ?
"Duplication of side effects" is one catch of macros that PEP 670 tries to avoid: https://peps.python.org/pep-0670/#rationale
Sadly, Py_CLEAR() cannot be converted to a function (as part of PEP 670) since it magically gets a reference to a pointer thanks to magic macro preprocessor. An hypothetical Py_Clear() function would take a pointer to a Python object, so
PyObject**type, like:Py_Clear(&variable);.IMO using
&argumentin the macro implementation is an acceptable fix to prevent the duplication of side effects.I think the compiler will optimize out the additional temporary variable in most cases.
I agree with you. Moreover, correctness matters more than performance for Py_CLEAR() API.
Maybe a
Py_ClearRef(PyObject **)function shoud also be added?Somehow related discussions:
I am against extending ABI without need. Py_CLEAR is a part of API, but not in ABI. Incref/decref is anough, Py_CLEAR is just an API sugar.
Calling
Py_Clear(&variable)can prevent some compiler optimizations. The compiler can no longer use a register forvariablein a register, and it can no longer assumevariableis now NULL, and it cannot guarantee the the value of localvariablewill not change outside of the local code. I faced a similar situation recently. And the problem was not only that the compiler generated less efficient code, but that it started issuing warnings about correct code that it could no longer fully analyze.How about this macro?
#define Py_CLEAR(op) \ do { \ PyObject **_py_tmp = (PyObject**)&(op); \ if (*_py_tmp != NULL) { \ PyObject* _py_tmp2 = *_py_tmp; \ *_py_tmp = NULL; \ /* Py_DECREF(_py_tmp2); */ \ free((void*)_py_tmp2); \ } \ } while (0)Replace the free line with the commented out line for real Python, this is for my earlier example program.
The macro Py_CLEAR(op) references the argument op two times. If the macro is called with an expression it will be evaluated two times, for example Py_CLEAR(p++).
This issue looks an hypothetical bug, but I'm not convinced that it's possible to write an expression which a side effect and which makes sense to set to NULL. For example, in your example, what's the point of writing
p++ = NULL;(through the macro).To be clear, GCC fails to build the following C code:
int main() { void *ptr = 0; ptr++ = 0; return 0; }I created PR #99100 to fix this issue. I'm not convinced that we should fix it, but a full PR might help to make a decision.
In the past, I saw surprising bug like https://bugs.python.org/issue43181 about passing a C++ expression to a C macro. So well, maybe in case of doubt, it's better to fix the issue to be extra safe. Py_CLEAR() pretends to be safer than using directly the Py_DECREF() macro ;-)
Ah wait, I read again #98724 (comment) and now I got the issue :-) I updated the unit test in my PR #99100.
Perfect! The main problem I have with macros that pretend to be functions is that they might evaluate their arguments several times, and this is totally unexpected. I hope the improved version works for all use-cases :)
I am not sure that it is not just a documentation issue. We can just document that the argument of Py_CLEAR (and the first argument of Py_SETREF) should not have side effect.
We can make it working even for arguments with a side effect, but should it be considered a bug fix or a new feature?
We can make it working even for arguments with a side effect, but should it be considered a bug fix or a new feature?
My PR fix the macro so it behaves correctly with arguments with side effects. For me it's a bugfix and should be backported to stable branches. If you are scared of the new implementation (using a pointer to a pointer), I'm fine with only changing the macro in Python 3.12 for now, and only backport if there is a strong pressure from many users to backport the fix.
24 remaining items
I am sad to say this: but looks like this feature broke one more thing :(
Since b11a384 we have this failure of
ARM64 Windows 3.x3114@vstinner tried to fixed it in cd67c1b but it did not work. And buildbots are still failing with this problem:
test_parse_in_error (test.test_ast.ASTHelpers_Test.test_parse_in_error) ... ok Windows fatal exception: stack overflow Current thread 0x00003664 (most recent call first): File "C:\Workspace\buildarea\3.x.linaro-win-arm64\build\Lib\test\test_ast.py", line 1252 in test_recursion_direct File "C:\Workspace\buildarea\3.x.linaro-win-arm64\build\Lib\unittest\case.py", line 579 in _callTestMethod File "C:\Workspace\buildarea\3.x.linaro-win-arm64\build\Lib\unittest\case.py", line 623 in run File "C:\Workspace\buildarea\3.x.linaro-win-arm64\build\Lib\unittest\case.py", line 678 in __call__ File "C:\Workspace\buildarea\3.x.linaro-win-arm64\build\Lib\unittest\suite.py", line 122 in run File "C:\Workspace\buildarea\3.x.linaro-win-arm64\build\Lib\unittest\suite.py", line 84 in __call__ File "C:\Workspace\buildarea\3.x.linaro-win-arm64\build\Lib\unittest\suite.py", line 122 in run File "C:\Workspace\buildarea\3.x.linaro-win-arm64\build\Lib\unittest\suite.py", line 84 in __call__ File "C:\Workspace\buildarea\3.x.linaro-win-arm64\build\Lib\unittest\suite.py", line 122 in run File "C:\Workspace\buildarea\3.x.linaro-win-arm64\build\Lib\unittest\suite.py", line 84 in __call__ File "C:\Workspace\buildarea\3.x.linaro-win-arm64\build\Lib\unittest\runner.py", line 208 in run File "C:\Workspace\buildarea\3.x.linaro-win-arm64\build\Lib\test\support\__init__.py", line 1100 in _run_suite File "C:\Workspace\buildarea\3.x.linaro-win-arm64\build\Lib\test\support\__init__.py", line 1226 in run_unittest File "C:\Workspace\buildarea\3.x.linaro-win-arm64\build\Lib\test\libregrtest\runtest.py", line 281 in _test_module File "C:\Workspace\buildarea\3.x.linaro-win-arm64\build\Lib\test\libregrtest\runtest.py", line 317 in _runtest_inner2 File "C:\Workspace\buildarea\3.x.linaro-win-arm64\build\Lib\test\libregrtest\runtest.py", line 360 in _runtest_inner File "C:\Workspace\buildarea\3.x.linaro-win-arm64\build\Lib\test\libregrtest\runtest.py", line 235 in _runtest File "C:\Workspace\buildarea\3.x.linaro-win-arm64\build\Lib\test\libregrtest\runtest.py", line 265 in runtest File "C:\Workspace\buildarea\3.x.linaro-win-arm64\build\Lib\test\libregrtest\main.py", line 352 in rerun_failed_tests File "C:\Workspace\buildarea\3.x.linaro-win-arm64\build\Lib\test\libregrtest\main.py", line 754 in _main File "C:\Workspace\buildarea\3.x.linaro-win-arm64\build\Lib\test\libregrtest\main.py", line 709 in main File "C:\Workspace\buildarea\3.x.linaro-win-arm64\build\Lib\test\libregrtest\main.py", line 773 in main File "C:\Workspace\buildarea\3.x.linaro-win-arm64\build\Lib\test\__main__.py", line 2 in <module> File "C:\Workspace\buildarea\3.x.linaro-win-arm64\build\Lib\runpy.py", line 88 in _run_code File "C:\Workspace\buildarea\3.x.linaro-win-arm64\build\Lib\runpy.py", line 198 in _run_module_as_main test_recursion_direct (test.test_ast.ASTHelpers_Test.test_recursion_direct) ...Is it really related? Or just a coincedence? 🤔
File "C:\Workspace\buildarea\3.x.linaro-win-arm64\build\Lib\test\test_ast.py", line 1252 in test_recursion_direct
I fixed the issue with commit cd67c1b.
Is it really related? Or just a coincedence? thinking
Tests on recursive functions are fragile, unless they use
support.infinite_recursion(). Depending how the Python binary is built, the stack memory used by each function call can vary a lot: compiler flags, function inlined or not, etc.On Windows, MSVC doesn't inline "static inline functions" in debug mode:
Capturing a few comments I made on Discord: I think it's a mistake to change the behaviour of
Py_CLEARto fix this issue. It's surprising behaviour, but changing the behaviour does not improve the situation and it may break existing code that relies on this behaviour. The common use ofPy_CLEARis with simple names or simple dereferences, which aren't affected by the surprising behaviour. Fixing the surprising corner case is not worth the change in semantics (and the added complexity of the implementation).I reopen the issue. The limited C API of Python 3.12 is still broken on compilers which don't provide
typeof(): Py_CLEAR() now requiresmemset()which is not provided by<Python.h>: thePython.hheader doesn't include the<string.h>header on purpose.Either the change is reverted to keep the bug on purpose, or the limited C API must be fixed.
Minimum extension module code reproducing the build error:
#define Py_LIMITED_API 0x030b0000 #include "Python.h" static int xx_modexec(PyObject *m) { PyObject *o = PyLong_FromLong(1); Py_CLEAR(o); return 0; } static PyModuleDef_Slot xx_slots[] = { {Py_mod_exec, xx_modexec}, {0, NULL} }; static struct PyModuleDef xxmodule = { PyModuleDef_HEAD_INIT, .m_name = "xxlimited", .m_size = 0, .m_slots = xx_slots, }; PyMODINIT_FUNC PyInit_xxlimited(void) { return PyModuleDef_Init(&xxmodule); }
Python is built with
-Werror=implicit-function-declarationand so building the extension fails with:gcc -fno-strict-overflow -Wno-unused-result -fno-omit-frame-pointer -mno-omit-leaf-frame-pointer -g -Og -Wall -O0 -std=c11 -Wno-unused-result -Werror=implicit-function-declaration -fvisibility=hidden -I./Include/internal -I. -I./Include -fPIC -c ./Modules/xxlimited.c -o Modules/xxlimited.o In file included from ./Include/Python.h:44, from ./Modules/xxlimited.c:3: ./Modules/xxlimited.c: In function 'xx_modexec': ./Include/object.h:651:13: error: implicit declaration of function 'memcpy' [-Werror=implicit-function-declaration] 651 | memcpy(_tmp_op_ptr, &_null_ptr, sizeof(PyObject*)); \ | ^~~~~~ ./Modules/xxlimited.c:9:5: note: in expansion of macro 'Py_CLEAR' 9 | Py_CLEAR(o); | ^~~~~~~~ (...) cc1: some warnings being treated as errorsBy the way, in LLVM 16, clang now treats this warning as an error by default:
- "The -Wimplicit-function-declaration and -Wimplicit-int warnings now default to an error in C99, C11, and C17. As of C2x, support for implicit function declarations and implicit int has been removed, and the warning options will have no effect. Specifying -Wimplicit-int in C89 mode will now issue warnings instead of being a noop."
- https://releases.llvm.org/16.0.0/tools/clang/docs/ReleaseNotes.html#potentially-breaking-changes
The problem of implicit function declaration is also becoming a major problem to migrate the C ecosystem to C99 and newer, see: https://fedoraproject.org/wiki/Changes/PortingToModernC
I created a thread on discuss about this issue: https://discuss.python.org/t/c-api-design-goal-hide-implementation-details-convert-macros-to-regular-functions-avoid-memset-or-errno-in-header-files-etc/25137
Maybe I missed it, but has anyone proposed yet to mix a macro (that takes the address of the argument and passes it into an inline function) with an inline function (that receives the pointer and does all the rest)?
I proposed multiple times to add functions rather than macros. Examples:
- 2017: https://groups.google.com/g/dev-python/c/RoeNq8Ind4U
- 2020: [WIP] bpo-42294: Add Py_SetRef() and Py_XSetRef() #23209
- 2022: Macro Py_CLEAR references argument two times. #98724 (comment)
@scoder's idea is a little bit different: the added function doesn't have to be part of the public C API.
@scoder: Do you think that such function should be public? Do you see advantages of a function rather than using a macro? I have my own rationale, but as you saw, I failed to convinced other people :-)
Accepted PEP 670 suggests converting macros to functions to they can be used in programs which cannot use macros.
I'm very late to this unfortunately, but the fallback version of
Py_CLEARusingmemcpyseems risky at the least.There's no guarantee that
foo* q; memcpy(&q, (foo **)&p, sizeof(q))will give you the same result asfoo *q = (foo *)p. Draft C11 does say that "All pointers to structure types shall have the same representation [...] as each other" (6.2.5 ¶ 28), which may (I'm not sure) make this code valid when the argument toPy_CLEARis a structure pointer, but there's no such guarantee forvoid *orchar *. In C++, casts between pointers to related struct/class types can add an offset, andmemcpying won't add that offset.It's probably not good practice to call
Py_CLEARon avoid *, and on most implementations it'd probably work anyway, and it may not be possible to multiple-inherit fromPyObjectfor other reasons, but it all seems risky. It's technically undefined and it could bite you.I think it'd be good to drop the guarantee of single evaluation from the documentation, because it can't be implemented portably. (Consider also that
memcpymay make for noticeably slower debug builds, and if people start writing code that depends on single evaluation then that regression would become unfixable.)You could use a template function when compiling as C++ (or
decltypeorautoif you don't care about pre-C++11; modern MSVC supports both), else__typeof__if available, else give up and do it the old way.I disagree with converting Py_CLEAR to regular function at first place. It was designed as a macro, and worked as a macro all these years. Your cure is worse than the (imaginary) disease.
I also think that we're trying to fix a non-problem here.
Py_CLEAR()is literally just a convenience macro. If the usage is not convenient for users, they don't need to (and probably should not) use it.I disagree with converting Py_CLEAR to regular function at first place.
I'm not sure you mean my suggestion of using a template function, but the template function would be defined in the header and always inlined when optimizing, so it shouldn't affect other optimizations or warnings (which was your concern in an earlier comment). And of course
Py_CLEARcould still be a macro that called it.But thinking about it again, if single evaluation can't be guaranteed anyway in the spec (and I think it shouldn't be) then it would probably be best to just revert to the old implementation.
I close the issue. The initial issue has been fixed. The fix landed in Python 3.12.0.
The Py_CLEAR() and Py_SETREF() macros are now implemented with
typeof()to avoid the duplication of side effects of using the same argument multiple times. The argument is now read exactly once. There is a fallback implementation using memcpy() when typeof() is not available (MSVC).
Bug report
The macro
Py_CLEAR(op)references the argumentoptwo times. If the macro is called with an expression it will be evaluated two times, for examplePy_CLEAR(p++).Your environment
x86_64
Python 3.7m
Debian Stable
I suggest a fix similar to this (old version commented out with #if 0):
I am not sure if this has happened anywhere, but I see a possible problem here. I think the compiler will optimize out the additional temporary variable in most cases.