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']

[gpshead]

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.

[gpshead]

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.

[colesbury]

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

[vstinner]

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)

[gpshead]

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