Skip to content

Possible UB in bytes_characters #106693

Open

Description

@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

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

and

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

Activity

  1. godlygeek commented on Jul 12, 2023

    @godlygeek
    Contributor

    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.

  2. added 2 commits that reference this issue on Jul 17, 2023
  3. ericsnowcurrently commented on Jul 17, 2023

    @ericsnowcurrently
    Member

    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?

  4. pablogsal commented on Jul 17, 2023

    @pablogsal
    MemberAuthor

    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

  5. markshannon commented on Aug 1, 2023

    @markshannon
    Member

    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)
  6. markshannon commented on Aug 1, 2023

    @markshannon
    Member

    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.

  7. added a commit that references this issue on May 1, 2024
  8. added a commit that references this issue on May 7, 2026
  9. added a commit that references this issue on May 7, 2026
  10. serhiy-storchaka commented on Jun 28, 2026

    @serhiy-storchaka
    Member

    The revert in #149514 (commit f0daba1) overreached: besides reverting the ob_sval change in Include/cpython/bytesobject.h, it also reverted gh-79638 ("Treat an unreachable robots.txt as 'disallow all'"), which is unrelated.

    It backed out the read() 5xx→disallow_all branch in Lib/urllib/robotparser.py, the corresponding tests in Lib/test/test_robotparser.py, and the gh-issue-79638 NEWS entry.

    gh-79638 had landed in 310fe88 immediately before, so the revert was likely cut from a stale base.

  11. added 4 commits that reference this issue on Jul 5, 2026
  12. added a commit that references this issue on Aug 8, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Metadata

Metadata

Labels

Projects

Milestone

No milestone

Relationships

None yet

Development

No branches or pull requests

Issue actions