Repository navigation
[mypyc] Fix memory safety bugs in librt.strings.BytesWriter - #22463
Open
Jahongir-Qurbonov wants to merge 1 commit into
Open
Jahongir-Qurbonov wants to merge 1 commit into
Jahongir-Qurbonov wants to merge 1 commit into
Conversation
- 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.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Two memory safety bugs in
librt.strings.BytesWriter.__setitem__writes out of bounds if converting the value changes the writerBoth the compiled path (
translate_bytes_writer_set_item) and the generic one (BytesWriter_ass_item) checked the index and then converted the value tou8. The conversion calls__index__for objects that aren'tint, and__index__can change the writer, e.g. callw.__init__(), which resets it to the small embedded buffer. The byte was then written at the old index: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] = 256on a 1-byte writer raisesTypeError, notIndexError).write(bytearray)races with a resize on free-threaded buildsCPyBytesWriter_WriteandBytesWriter_write_internalread 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 andCPyByteArray_SetItemdo. 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):writewriteI first tried
PyObject_GetBufferinstead of the critical section, but it made compiledwrite(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. Fullmypyctest suite passes.