Repository navigation
Undefined C behavior going beyond end of struct via a [1] arrays (C99 flexible arrays) #84301
Description
Activity
The correct C99 way to do this is using a char[]. PyBytesObject and unicode's struct encoding_map both do this.
Unclear to me if we should backport this to earlier versions or not (because PyBytesObject may be exposed?) Probably, but I also doubt it is a big deal as compilers are generally not _yet_ making use of this detail AFAIK.
From the PR comment thread (as I opened that first):
"""Well, there was no other choice in ISO C89 than using char ob_sval[1];, no?
Is char ob_sval[]; supported by the C compiler supported by CPython? Like Visual Studio, GCC, clang and xlc (AIX)? (I don't think that we officially support xlc, but it's more "best effort" support.)
You can use the new buildbot label to test you change on more platforms.""" - vstinner
Per https://www.python.org/dev/peps/pep-0007/ we require some C99 features as of CPython 3.6. It does not currently list Flexible array member.
I'll be very surprised if we find any compiler that does not support this.
I'll run this through the buildbot testing as you suggested and assuming nothing important falls out, see that we add this to the C99 required feature list.
It may be worth considering C-API extensions written in C++. Flexible array members are not part of the C++ standard, although GCC, Clang, and MSVC support them as an extension. GCC and Clang will issue warnings with
-Wpedanticand MSVC will issue warnings with/Wall. Currently, C++ code that includes<Python.h>is warning-free in GCC (g++) even with-Wpedantic. That won't be true after this change, unless Py_LIMITED_API is defined.Note that GCC also explicitly supports trailing one-element arrays (the current pattern) as an extension. https://gcc.gnu.org/onlinedocs/gcc/Zero-Length.html
Currently, C++ code that includes
<Python.h>is warning-free in GCC (g++) even with-Wpedantic. That won't be true after this change, unless Py_LIMITED_API is defined.By the way, the current trend is to make more and more structures opaque in the C API. See for example:
"""
The [1] thing is used in more places:PyObject *f_localsplus[1]; in PyFrameObject
PyObject *ob_item[1]; in PyTupleObject
void *elements[1]; in asdl_seq and int elements[1];` in asdl_int_seq
digit ob_digit[1]; in PyLongObject
Py_ssize_t ob_array[1]; in PyMemoryViewObject
_tracemalloc.c: frame_t frames[1]; in traceback_tDo we need to update all of them? Do you want to update them all?
""" - vstinnerI believe we probably do. But I suggest multiple PRs. I'll update the issue title.
I'm also going to ask clang/llvm folks that prompted me to look into this for comments.
- changed the title
[-]Undefined C behavior going beyond end of struct via a char[1].[/-][+]Undefined C behavior going beyond end of struct via a [1] arrays.[/+]on Mar 31, 2020 - changed the title
[-]Undefined C behavior going beyond end of struct via a char[1].[/-][+]Undefined C behavior going beyond end of struct via a [1] arrays.[/+]on Mar 31, 2020 16 remaining items
How about:
#ifdef __cplusplus char array[1]; #else char array[]; #endif
?
#ifdef __cplusplus char array[1]; #else char array[]; #endif
Does it change the size of the structure between C and C++, sizeof(PyBytesObject)? Also, does the size of the struture matter? :-) I guess that the impart part is the ABI: offset of the members.
off the top of my head that might actually work as I _think_ "empty" things are required to consume an unused byte of size no matter what meaning sizeof shouldn't change? Some testing and standards perusing for C99 is in order to confirm that though.
- changed the title
[-]Undefined C behavior going beyond end of struct via a [1] arrays.[/-][+]Undefined C behavior going beyond end of struct via a [1] arrays (flexible arrays)[/+]on Jun 7, 2022 - changed the title
[-]Undefined C behavior going beyond end of struct via a [1] arrays (flexible arrays)[/-][+]Undefined C behavior going beyond end of struct via a [1] arrays (C99 flexible arrays)[/+]on Jun 7, 2022 (Coming from #94250 which was marked as a duplicate) Bump.
Error: ---
$ LANG= g++ -pedantic -Werror x.cpp x.c:14:10: error: ISO C++ forbids flexible array member 'array' [-Werror=pedantic] 14 | char array[]; | ^~~~~ cc1plus: all warnings being treated as errors
The g++
-pedanticand MSVC C4200 warnings (not seen in clang or Intel C++) can be suppressed.#ifdef __GNUC__ #pragma GCC diagnostic push #pragma GCC diagnostic ignored "-Wpedantic" #elif defined(_MSC_VER) #pragma warning(push) #pragma warning(disable:4200) // nonstandard extension used: zero-sized array in struct/union #endif typedef struct { int x; int y; char array[]; } mystruct_t; #ifdef __GNUC #pragma GCC diagnostic pop #elif defined(_MSC_VER) #pragma warning(pop) #endif
clang
-fsanitize=array-boundscurrently has a hack to work around projects like CPython. It'd be nice if the hack can be removed. AIUI, the Clang (possibly also GCC)'s trend is to eventually drop the workaround for the pseudo flexible array member.Reacted by Gregory P. SmithGCC
-Warray-bounds=2flags this kind of UB.In Python 3.12, there are many bytes objects which are initialized statically in Include/internal/pycore_runtime_init.h. Problem: if PyBytesObjects ends with
char ob_sval[],ob_svalcan not be initialized statically in a portable way.One option is to declare a different but similar structure ending with
char ob_sval[3], initializeob_sval(to 3 bytes), and then cast to the expected type (ex:PyBytesObject).It seems like such cast is not an undefined behavior.
Full example:
// clang x.c -o x -O2 -fsanitize=undefined && ./x // gcc x.c -o x -O2 -fsanitize=undefined && ./x #include <stdio.h> typedef struct { int size; char ob_sval[]; } PyBytesObject; int main() { struct { int size; char ob_sval[3]; } raw_abc = { .size = 1, .ob_sval = { 'a', 'b', 'c' } }; PyBytesObject *abc = (PyBytesObject *)&raw_abc; printf("bytes: \"%c%c%c\"\n", abc->ob_sval[0], abc->ob_sval[1], abc->ob_sval[2]); return 0; }
Test with gcc (GCC) 12.1.1 and clang version 14.0.0:
$ clang x.c -o x -O2 -fsanitize=undefined && ./x bytes: "abc" $ gcc x.c -o x -O2 -fsanitize=undefined && ./x bytes: "abc"Note: GCC has an extension allowing to initialize
char ob_sval[]to{'a', 'b', 'c'}(3 bytes) for example. But it doesn't work with clang.Reacted by Gregory P. SmithOne option is to declare a different but similar structure ending with char ob_sval[3], initialize ob_sval (to 3 bytes), and then cast to the expected type (ex: PyBytesObject).
This is already done for statically initializing ascii strings (identifiers) and it works.
LPC 2022 "Where are we on security features?" is related to this. https://www.youtube.com/watch?v=L2Ydq3iFwsY&t=5550s mentions a
__attribute__((element_count(size)))proposal which can let the compiler know the array size.The Linux kernel has a similar issue with flexible arrays: https://lwn.net/Articles/908817/
I proposed PR #97017 to convert PyBytes_AS_STRING() static inline function to a regular function, it would open the ability to make the PyBytesObject structure opaque and avoids the issue in the public C API at least. But this PR might be rejected because of its impact on performance.
FWIW, the new version of the openssf compiler hardening guide is now recommending use of
-fstrict-flex-arrays=3https://best.openssf.org/Compiler-Hardening-Guides/Compiler-Options-Hardening-Guide-for-C-and-C++.html#enable-strict-flexible-arrays, which obviously stumbles on most things using libpython.Reacted by Gregory P. Smith
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: