Skip to content

[mypyc] Fix memory safety bugs in librt.strings.BytesWriter - #22463

Open
Jahongir-Qurbonov wants to merge 1 commit into
python:masterfrom
Jahongir-Qurbonov:mypyc-byteswriter-memory-safety
Open

Jahongir-Qurbonov wants to merge 1 commit into
python:masterfrom
Jahongir-Qurbonov:mypyc-byteswriter-memory-safety

Conversation

@Jahongir-Qurbonov

Copy link
Copy Markdown
Contributor

Two memory safety bugs in librt.strings.BytesWriter.

__setitem__ writes out of bounds if converting the value changes the writer

Both the compiled path (translate_bytes_writer_set_item) and the generic one (BytesWriter_ass_item) checked the index and then converted the value to u8. The conversion calls __index__ for objects that aren't int, and __index__ can change the writer, e.g. call w.__init__(), which resets it to the small embedded buffer. The byte was then written at the old index:

class Reset:
    def __init__(self, w): self.w = w
    def __index__(self):
        self.w.__init__()
        return 0x41

w = BytesWriter()
w.write(b"x" * 100000)
w[99999] = Reset(w)  # writes past the 256-byte embedded buffer

Now both paths convert the index and the value first, then check the bounds against the current length. This also means that, like bytearray, an invalid value is reported before an invalid index (w[5] = 256 on a 1-byte writer raises TypeError, not IndexError).

write(bytearray) races with a resize on free-threaded builds

CPyBytesWriter_Write and BytesWriter_write_internal read the buffer pointer and size of the bytearray without locking it. On free-threaded builds another thread can resize the bytearray during the copy, so we read freed memory. This PR locks the bytearray with a critical section, like CPython's bytearray methods and CPyByteArray_SetItem do. On builds with the GIL this is a no-op.

Stress test on 3.14t (-X gil=0): three threads write a shared 100 KB bytearray into new writers while another thread resizes it (100000 writes per thread):

interpreted write compiled write
before 8/8 runs segfault 3/8 runs segfault
after 0/8 0/8

I first tried PyObject_GetBuffer instead of the critical section, but it made compiled write(bytearray) ~2.5x slower on GIL builds. With the critical section, 5M writes of a 4- or 64-byte bytearray take the same time as before (16 ms / 21 ms on 3.13).

Tests

New run tests for __setitem__ with a value whose __index__ resets the writer (compiled and generic path), for the order of the checks, and for writing a bytearray subclass. The __setitem__ tests fail without the fix. The free-threaded race isn't tested automatically, since it's timing-dependent. Full mypyc test suite passes.

- BytesWriter.__setitem__ checked the index before converting the value.
  The conversion can call __index__, which can reset the writer to its
  small initial buffer, so the byte was written out of bounds. Convert
  the value (and the index) before the bounds check, in both the
  compiled and the generic path. Like bytearray, an invalid value is now
  reported before an invalid index.

- BytesWriter.write(bytearray) read the buffer and size of the
  bytearray without locking it. On free-threaded builds another thread
  could resize the bytearray during the copy. Lock it with a critical
  section, like the bytearray methods do. This is a no-op on builds
  with the GIL.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant