Undefined C behavior going beyond end of struct via a [1] arrays (C99 flexible arrays)

@gpshead

BPO	40120
Nosy	@gpshead, @pitrou, @vstinner, @serhiy-storchaka, @colesbury, @pablogsal
PRs	bpo-40120: Fix unbounded struct char[] undefined behavior. #19232

^{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:

assignee = 'https://github.com/gpshead'
closed_at = None
created_at = <Date 2020-03-30.21:04:23.697>
labels = ['interpreter-core', 'build', '3.11']
title = 'Undefined C behavior going beyond end of struct via a [1] arrays.'
updated_at = <Date 2022-01-21.23:32:19.727>
user = 'https://github.com/gpshead'

bugs.python.org fields:

activity = <Date 2022-01-21.23:32:19.727>
actor = 'gregory.p.smith'
assignee = 'gregory.p.smith'
closed = False
closed_date = None
closer = None
components = ['Interpreter Core']
creation = <Date 2020-03-30.21:04:23.697>
creator = 'gregory.p.smith'
dependencies = []
files = []
hgrepos = []
issue_num = 40120
keywords = ['patch']
message_count = 20.0
messages = ['365349', '365350', '365399', '365403', '365410', '365415', '365425', '365464', '365465', '365466', '365472', '365539', '365671', '365691', '365744', '365745', '366883', '411151', '411168', '411207']
nosy_count = 6.0
nosy_names = ['gregory.p.smith', 'pitrou', 'vstinner', 'serhiy.storchaka', 'colesbury', 'pablogsal']
pr_nums = ['19232']
priority = 'normal'
resolution = None
stage = 'needs patch'
status = 'open'
superseder = None
type = 'compile error'
url = 'https://bugs.python.org/issue40120'
versions = ['Python 3.11']

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 -Wpedantic and 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:

bpo-39573: Make PyObject an opaque structure in the limited C API
bpo-39947: Make the PyThreadState structure opaque (move it to the internal C API)

"""
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_t

Do we need to update all of them? Do you want to update them all?
""" - vstinner

I 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.

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.

See also issue #93585 "PyCode_Type.tp_basictype change in Python 3.11 broke Cython".

(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++ -pedantic and 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-bounds currently 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.

GCC -Warray-bounds=2 flags 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_sval can not be initialized statically in a portable way.

One 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).

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.

One 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=3 https://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.

Navigation Menu

Repository navigation

Undefined C behavior going beyond end of struct via a [1] arrays (C99 flexible arrays) #84301

16 remaining items

Metadata

Assignees

Labels

Projects

Milestone

Relationships

Development

Undefined C behavior going beyond end of struct via a [1] arrays (C99 flexible arrays) #84301

Description

Activity

gpshead commented on Mar 30, 2020

gpshead commented on Mar 30, 2020

colesbury commented on Mar 31, 2020

vstinner commented on Mar 31, 2020

gpshead commented on Mar 31, 2020

16 remaining items

pitrou commented on Jan 21, 2022

vstinner commented on Jan 21, 2022

gpshead commented on Jan 21, 2022

vstinner commented on Jun 7, 2022

MaskRay commented on Jun 27, 2022

MaskRay commented on Jun 30, 2022

vstinner commented on Jul 4, 2022

kumaraditya303 commented on Jul 4, 2022

MaskRay commented on Sep 19, 2022

vstinner commented on Sep 27, 2022

risicle commented on Apr 6, 2025

Metadata

Metadata

Assignees

Labels

Projects

Milestone

Relationships

Development

Issue actions