python / cpython

The Python programming language
https://www.python.org
Other
62.52k stars 30.01k forks source link

Reliance on C bit fields in C API is undefined behavior #89188

Open 236914ab-5504-4492-add8-6907a1140f5c opened 3 years ago

236914ab-5504-4492-add8-6907a1140f5c commented 3 years ago
BPO 45025
Nosy @birkenfeld, @vstinner, @encukou, @methane, @serhiy-storchaka, @indygreg

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: ```python assignee = None closed_at = None created_at = labels = ['type-bug', '3.8', '3.9', '3.10', '3.11', 'expert-C-API', '3.7'] title = 'Reliance on C bit fields in C API is undefined behavior' updated_at = user = 'https://github.com/indygreg' ``` bugs.python.org fields: ```python activity = actor = 'vstinner' assignee = 'none' closed = False closed_date = None closer = None components = ['C API'] creation = creator = 'indygreg' dependencies = [] files = [] hgrepos = [] issue_num = 45025 keywords = [] message_count = 9.0 messages = ['400388', '400615', '400617', '400620', '400621', '400622', '400624', '400634', '400788'] nosy_count = 6.0 nosy_names = ['georg.brandl', 'vstinner', 'petr.viktorin', 'methane', 'serhiy.storchaka', 'indygreg'] pr_nums = [] priority = 'normal' resolution = None stage = None status = 'open' superseder = None type = 'behavior' url = 'https://bugs.python.org/issue45025' versions = ['Python 3.6', 'Python 3.7', 'Python 3.8', 'Python 3.9', 'Python 3.10', 'Python 3.11'] ```

Linked PRs

236914ab-5504-4492-add8-6907a1140f5c commented 3 years ago

At least the PyASCIIObject struct in Include/cpython/unicodeobject.h uses bit fields. Various preprocessor macros like PyUnicode_IS_ASCII() and PyUnicode_KIND() access this struct's bit field.

This is problematic because according to the C specification, the storage of bit fields is unspecified and may vary from compiler to compiler or architecture to architecture. Theoretically, a build of libpython with compiler A may not have the same storage layout of a bit field as a separate binary built with compiler B. These 2 binaries could be linked/loaded together, resulting in a crash or incorrect behavior at run-time.

https://stackoverflow.com/questions/6043483/why-bit-endianness-is-an-issue-in-bitfields/6044223#6044223

To ensure bit field behavior is consistent, the same compiler must be used for all bit field interaction. Since it is effectively impossible to ensure this for programs like Python where multiple compilers are commonly at play (a 3rd party C extension will likely not be built on the same machine that built libpython), bit fields must not be exposed in the C API. If a bit field must exist, the bit field should not be declared in a public .h header and any APIs for accessing the bit field must be implemented as compiled functions such that only a single compiler will define the bit field storage layout.

In order to avoid undefined behavior, Python's C API should avoid all use of bit fields.

This issue is in response to https://github.com/PyO3/pyo3/issues/1824.

vstinner commented 3 years ago

At least the PyASCIIObject struct in Include/cpython/unicodeobject.h uses bit fields. Various preprocessor macros like PyUnicode_IS_ASCII() and PyUnicode_KIND() access this struct's bit field.

What is your use case? Which functions do you need?

You should not access directly the PyASCIIObject structure. Python provides many functions to access the content of a Unicode string object.

encukou commented 3 years ago

The macro PyUnicode_KIND is part of the documented public C API. It accesses the bit field "state.kind" directly.

vstinner commented 3 years ago

The macro PyUnicode_KIND is part of the documented public C API.

IMO it was a mistake to expose it as part of the public C API. This is an implementation detail which should not be exposed. The C API should not expose *directly* how characters are stored in memory, but provide an abstract way to read and write Unicode characters.

The PEP-393 implementation broke the old C API in many ways because it exposed too many implementation details. Sadly, the new C API is... not better :-(

If tomorrow, CPython is modified to use UTF-8 internally (as PyPy does), the C API will likely be broken *again* in many (new funny) ways.

11 years after the PEP-393 (Python 3.3), we only start fixing the old C API :-( The work will be completed in 2 or 3 Python releases (Python 3.12 or 3.13):

The C API for Unicode strings is causing a lot of issues in PyPy which uses UTF-8 internally. C extensions can fail to build on PyPy if they use functions (macros) like PyUnicode_KIND().

vstinner commented 3 years ago

In order to avoid undefined behavior, Python's C API should avoid all use of bit fields.

See also the PEP-620. IMO more generally, the C API should not expose structures, but provide ways to access it through getter and setter functions.

See bpo-40120 "Undefined C behavior going beyond end of struct via a [1] arrays" which is a similar issue.

encukou commented 3 years ago

PyUnicode_KIND does *not* expose the implementation details to the programmer.

If the internal representation os strings is switched to use masks and shifts instead of bitfields, PyUnicode_KIND (and others) can be adapted to the new details without breaking API compatibility. And that switch would fix this issue.

vstinner commented 3 years ago

PyUnicode_KIND does *not* expose the implementation details to the programmer.

PyUnicode_KIND() is very specific to the exact PEP-393 implementation. Documentation of this field: --- /* Character size:

     * character type = Py_UCS4 (32 bits, unsigned)
     * all characters are in the range U+0000-U+10FFFF
     * at least one character is in the range U+10000-U+10FFFF
 */
unsigned int kind:3;

I don't think that PyUnicode_KIND() makes sense if CPython uses UTF-8 tomorrow.

If the internal representation os strings is switched to use masks and shifts instead of bitfields, PyUnicode_KIND (and others) can be adapted to the new details without breaking API compatibility.

PyUnicode_KIND() was exposed in the *public* C API because unicodeobject.h provides functions as macros for best performances, and these macros use PyUnicode_KIND() internally.

Macros like PyUnicode_READ(kind, data, index) are also designed for best performances with the exact PEP-393 implementation.

The public C API should only contain PyUnicode_READ_CHAR(unicode, index): this macro doesn't use "kind" or "data" which are (again) specific to the PEP-393.

In the CPython implementation, we should use the most efficient code, it's fine to use macros accessing directly structures.

But for the public C API, I would recommend to only provide abstractions, even if there are a little bit slower.

236914ab-5504-4492-add8-6907a1140f5c commented 3 years ago

My use case for these low-level APIs is to write tests for low-level string/encoding handling in my custom use of the PyPreConfig and PyConfig structs. I wanted to verify that exact byte sequences were turned into specific representations inside of Python strings. This includes ensuring that certain byte sequences retain their appropriate "character" width in internal storage.

I know there are alternative ways of performing this testing. But testing against the actual data structure used internally by CPython seemed the most precise since it isolates problems to the "store in Python" side of the problem and not "what does Python do once the data is stored."

vstinner commented 3 years ago

My use case for these low-level APIs is to write tests for low-level string/encoding handling in my custom use of the PyPreConfig and PyConfig structs. I wanted to verify that exact byte sequences were turned into specific representations inside of Python strings. This includes ensuring that certain byte sequences retain their appropriate "character" width in internal storage.

CPython contains many checks to ensure that a string always use the most effecient storage, especially in debug mode. The C API should not allow to create a string using an inefficient storage, unless you "abuse" the C API :-D

I'm not sure what do you test.

ericsnowcurrently commented 6 months ago

(Also see the discussion between @davidhewitt and @encukou at https://www.youtube.com/watch?v=E_hczlgDqus.)

vstinner commented 6 months ago

See also https://github.com/capi-workgroup/api-evolution/issues/10