bpo-40120: Fix unbounded struct char[] undefined behavior. by gpshead · Pull Request #19232 · python/cpython

gpshead · 2020-03-30T20:15:53Z

It is undefined behaviour if index is beyond array size. The workaround
for this is the standard C99 feature known as "Flexible array member".

https://en.wikipedia.org/wiki/Flexible_array_member

https://bugs.python.org/issue40120

It is undefined behaviour if index is beyond array size. The workaround for this is standard C99 feature known as "Flexible array member". https://en.wikipedia.org/wiki/Flexible_array_member

gpshead · 2020-03-30T20:16:26Z

(yes a BPO issue is needed, i'm starting with the PR to see how the CI bots fare)

gpshead · 2020-03-30T20:18:22Z

It looks like we thankfully got rid of any use of sizeof(PyBytesObject) a long time ago (which never really made much sense anyways) as part of https://bugs.python.org/issue4445 so I believe this should have no negative consequences.

gpshead · 2020-03-30T20:25:22Z

There are other instances of this pattern in our codebase. Objects/unicodeobject.c has encoding_map who's level32 member appears to use this? Grepping I don't immediately see others.

This one was found by a coworker working to enable clang analyzer.

vstinner · 2020-03-30T21:05:10Z

It is undefined behaviour if index is beyond array size.

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.

gpshead · 2020-03-30T21:14:44Z

Moving discussion to the bug. (TL;DR I will do the buildbot tests and add this feature to PEP 7 unless we find an important platform that balks about it). Not being explicit in PEP7 yet suggests this should not be backported.

This is really nothing more than a change test, it needs to match the C struct when no level23 mappings will be allocated. An empty char[] does not consume any C sizeof() space whereas the previous `char[1]` consumed one alignment size worth, even when unused.

bedevere-bot · 2020-03-30T22:39:08Z

🤖 New build scheduled with the buildbot fleet by @gpshead for commit cd483f6 🤖

If you want to schedule another build, you need to add the ":hammer: test-with-buildbots" label again.

vstinner · 2020-03-31T12:11:10Z

Objects/bytesobject.c

+   The + 1 accounts for the trailing \0 byte that we include as a safety
+   measure for code that treats the underlying char * as a C string.
 */
 #define PyBytesObject_SIZE (offsetof(PyBytesObject, ob_sval) + 1)


Wait, you made PyBytesObject 1 byte smaller, and you didn't have to substract 1 somewhere? Does it mean that Python overallocates 1 byte since forever?

Thankfully not. Because this code used offsetof the macro did not need to change. The offset never changes. The existing + 1 in the macro is easy to misinterpret as having been there to account for the [1] but that isn't how it was used - PyBytesObject is always allocated with that one extra byte that is filled with a NUL for C safety for PyBytes_AsString users.

vstinner

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 · 2020-03-31T12:17:10Z

Refleaks buildbots are likely caused by https://bugs.python.org/issue40115 (known leak)

kadler · 2020-11-14T00:13:19Z

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

Yep, this is supported by xlc.

vstinner · 2022-01-21T16:49:52Z

Objects/unicodeobject.c

    unsigned char level1[32];
    int count2, count3;
-    unsigned char level23[1];
+    unsigned char level23[];


@gpshead: Can you please write a PR which only contains this change? Since it doesn't touch the public C API, C++ compatibility is not an issue (I don't think that Python can be built with a C++ compiler). We can start with this change and see how it goes? See also the discussion at python/peps#1349

vstinner · 2022-01-21T16:51:30Z

Include/cpython/bytesobject.h

    PyObject_VAR_HEAD
    Py_hash_t ob_shash;
-    char ob_sval[1];
+    char ob_sval[];


I suggest to:

Leave the structure as it is

Use a code search to check which C extension access directly PyBytesObject structure members (well, first check Cython and pybind11)

Prepare affected C extensions

Once the number of affected C extensions is low enough: make the structure opaque, enforce the usage of public C functions.

gpshead · 2022-01-21T23:32:47Z

see the bug, a new PR can be made.

the-knights-who-say-ni added the CLA signed label Mar 30, 2020

bedevere-bot added the awaiting core review label Mar 30, 2020

gpshead requested a review from vstinner March 30, 2020 20:16

gpshead added 2 commits March 30, 2020 13:53

Fix unicode's struct encoding_map similarly.

d3329fe

Describe the + 1 used in PyBytesObject_SIZE.

a1759d9

gpshead changed the title ~~Fix undefined behavior in PyBytesObject.~~ bpo-40120: Fix undefined behavior in PyBytesObject. Mar 30, 2020

gpshead changed the title ~~bpo-40120: Fix undefined behavior in PyBytesObject.~~ bpo-40120: Fix unbounded struct char[] undefined behavior. Mar 30, 2020

news!

630a26a

gpshead self-assigned this Mar 30, 2020

gpshead added the 🔨 test-with-buildbots Test PR w/ buildbots; report in status section label Mar 30, 2020

bedevere-bot removed the 🔨 test-with-buildbots Test PR w/ buildbots; report in status section label Mar 30, 2020

gpshead mentioned this pull request Mar 31, 2020

PEP 007: Add C99 Flexible Array Member to the required feature list. python/peps#1349

Closed

vstinner reviewed Mar 31, 2020

View reviewed changes

gpshead added the DO-NOT-MERGE label Apr 3, 2020

vstinner reviewed Jan 21, 2022

View reviewed changes

gpshead closed this Jan 21, 2022

gpshead mentioned this pull request Apr 10, 2022

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

Open

Uh oh!

Conversation

gpshead commented Mar 30, 2020 • edited by bedevere-bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

gpshead commented Mar 30, 2020

Uh oh!

gpshead commented Mar 30, 2020

Uh oh!

gpshead commented Mar 30, 2020

Uh oh!

vstinner commented Mar 30, 2020

Uh oh!

gpshead commented Mar 30, 2020

Uh oh!

bedevere-bot commented Mar 30, 2020

Uh oh!

vstinner Mar 31, 2020

Choose a reason for hiding this comment

Uh oh!

gpshead Mar 31, 2020

Choose a reason for hiding this comment

Uh oh!

vstinner left a comment

Choose a reason for hiding this comment

Uh oh!

vstinner commented Mar 31, 2020

Uh oh!

kadler commented Nov 14, 2020

Uh oh!

vstinner Jan 21, 2022

Choose a reason for hiding this comment

Uh oh!

vstinner Jan 21, 2022

Choose a reason for hiding this comment

Uh oh!

gpshead commented Jan 21, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

gpshead commented Mar 30, 2020 •

edited by bedevere-bot

Loading