Possible UB in bytes_characters

[pablogsal]

in bytes_characters in https://github.com/python/cpython/blob/main/Include/internal/pycore_runtime_init.h we are doing this:

.bytes_characters = _Py_bytes_characters_INIT, \

where

#define _Py_bytes_characters_INIT { \
 _PyBytes_CHAR_INIT(0), \
 _PyBytes_CHAR_INIT(1), \
 _PyBytes_CHAR_INIT(2), \
 _PyBytes_CHAR_INIT(3), \
 _PyBytes_CHAR_INIT(4), \
 _PyBytes_CHAR_INIT(5), \
 _PyBytes_CHAR_INIT(6), \
 _PyBytes_CHAR_INIT(7), \
 _PyBytes_CHAR_INIT(8), \
 _PyBytes_CHAR_INIT(9), \
...
 _PyBytes_CHAR_INIT(255), \

with

#define _PyBytes_CHAR_INIT(CH) \
 { \
 _PyBytes_SIMPLE_INIT((CH), 1) \
 }

#define _PyBytes_SIMPLE_INIT(CH, LEN) \
 { \
 _PyVarObject_HEAD_INIT(&PyBytes_Type, (LEN)) \
 .ob_shash = -1, \
 .ob_sval = { (CH) }, \
 }

but

cpython/Include/internal/pycore_global_objects.h

Lines 41 to 44 in e4b88c1

	struct {
	PyBytesObject ob;
	char eos;
	} bytes_characters[256];

and

cpython/Include/cpython/bytesobject.h

Lines 5 to 15 in e4b88c1

	typedef struct {
	PyObject_VAR_HEAD
	Py_DEPRECATED(3.11) Py_hash_t ob_shash;
	char ob_sval[1];
	/* Invariants:
	* ob_sval contains space for 'ob_size+1' elements.
	* ob_sval[ob_size] == 0.
	* ob_shash is the hash of the byte string or -1 if not computed yet.
	*/
	} PyBytesObject;

I think this is UB because when we are basically assigning 255 to:

https://github.com/python/cpython/blob/e4b88c1e4ac129b36f99a534387d64f7b8cda8ef/Include/cpython/bytesobject.h#L8C10-L8C17

and that should be a unsigned char because the range of char can be 0 to 255 or -128 to 127 depending on the platform.

Linked PRs

• gh-106693: Explicitly mark ob_sval as unsigned char to avoid UB #106826
• [3.14] gh-106693: Explicitly mark ob_sval as unsigned char to avoid UB (GH-106826) #149510
• [3.13] gh-106693: Explicitly mark ob_sval as unsigned char to avoid UB (GH-106826) #149511
• gh-106693: Revert "Explicitly mark ob_sval as unsigned char to avoid UB (#106826)" #149514

[godlygeek]

Per C11:

The three types char, signed char, and unsigned char are collectively called the character types. The implementation shall define char to have the same range, representation, and behavior as either signed char or unsigned char.

With a footnote:

CHAR_MIN, defined in <limits.h>, will have one of the values 0 or SCHAR_MIN, and this can be used to distinguish the two options. Irrespective of the choice made, char is a separate type from the other two and is not compatible with either.

[ericsnowcurrently]

Doesn't the problem with "char" vs. "unsigned char" leak out everywhere else in the bytes-related C-API?

Is the problem here only that we're using a literal outside the range of "signed char"? If so, could we adjust the initializer for the single-byte bytes objects with a cast or using CHAR_MIN or something like that?

[pablogsal]

Doesn't the problem with "char" vs. "unsigned char" leak out everywhere else in the bytes-related C-API?

It does not unless you assign something bigger than 128 or something negative to it.

Is the problem here only that we're using a literal outside the range of "signed char"?

Exactly. We should restrict the initialisation to only the signed char range or change the type

[markshannon]

We should change the type to unsigned char or uint8_t

>>> bytes([0, 160])
b'\x00\xa0'
>>> bytes([-1, 1])
Traceback (most recent call last):
 File "<stdin>", line 1, in <module>
ValueError: bytes must be in range(0, 256)

[markshannon]

Doesn't the problem with "char" vs. "unsigned char" leak out everywhere else in the bytes-related C-API?

It shouldn't, but it will need a few casts in on entry or return to those functions.