gh-148286: Fix undefined behaviour in io.StringIO.read() after an overseek - #154913
gh-148286: Fix undefined behaviour in io.StringIO.read() after an overseek#154913matthiasgoergens wants to merge 4 commits into
Conversation
…an overseek
_io_StringIO_read_impl() clamps the read size to zero when the position is
past the end of the buffer, but still formed the pointer
output = self->buf + self->pos;
before returning the empty string. Nothing is read through that pointer,
but forming it is undefined behaviour on its own, and because self->buf is
a Py_UCS4 *, the byte offset is self->pos * 4: for self->pos == 2**62 - 1
the multiplication wraps and yields a pointer four bytes BELOW self->buf.
That is what UBSan reported:
io.StringIO("abc").seek(2**62 - 1) followed by .read()
stringio.c:352: addition of unsigned offset to 0x... overflowed to 0x...
Return the empty string early, exactly as _stringio_readline() already
does for the same overseek condition. self->pos += size is a no-op here
since size is zero, and ENSURE_REALIZED() is kept ahead of the guard so
the state transition and its error path are unchanged.
The existing test_io suite already covers this path, which is how UBSan
found it. The Modules/_io/stringio.c entry in Tools/ubsan/suppressions.txt
is no longer needed; with it removed, test_io passes under
UBSAN_OPTIONS=halt_on_error=1, where before it aborts at stringio.c:352.
987fb6a to
addc87f
Compare
| @@ -0,0 +1,7 @@ | |||
| Fix undefined behaviour in :meth:`!io.StringIO.read` after an overseek. | |||
There was a problem hiding this comment.
Same idea as with the ctypes PR, avoid implementation/infra details.
picnixz
left a comment
There was a problem hiding this comment.
Please add a regression test.
|
A Python core developer has requested some changes be made to your pull request before we can consider merging it. If you could please address their requests along with any other requests in other reviews from core developers that would be appreciated. Once you have made the requested changes, please leave a comment on this pull request containing the phrase |
Review feedback: picnixz asked for a regression test, StanFromIreland for the NEWS entry to drop implementation and infrastructure detail. The existing test_overseek only seeks one past the end, which is why it never exercised this: the pointer arithmetic only wraps for positions above PY_SSIZE_T_MAX // 4. The new test covers 2**31, 2**62 and sys.maxsize, and sits in MemoryTestMixin so it runs for BytesIO as well as StringIO, in both the C and pure-Python implementations. Verified that all four already satisfy it, which is the point -- it pins the documented behaviour rather than detecting the UB. NEWS entry cut to the user-visible statement.
|
Thanks both. Regression test — added in 0d659fa, as One thing worth being explicit about, because it affects how much the So the test pins the documented behaviour rather than detecting the What actually guards the fix is the other half of the diff: removing from NEWS entry — trimmed to the user-visible statement, no macro names, I have made the requested changes; please review again |
|
Thanks for making the requested changes! @picnixz: please review the changes made to this pull request. |
Verified under UBSan with CI's own configuration (--with-undefined-behavior-sanitizer --with-strict-overflow, clang): CStringIOTest.testSeek already triggers the report, because it does seek(sys.maxsize) followed by read(4), which is exactly the case where the pointer arithmetic wraps. That is what produced the diagnostic recorded in the suppressions file. My added test was therefore redundant for detection: small overseeks (11, 100, 2**31) produce no report at all, and the large offsets it used were already covered. Removed rather than kept as noise. Left a comment on testSeek instead, since its name gives no hint that those two lines guard a UB fix, and trimming them would silently remove the coverage.
|
Correction to my last comment, and a retraction: the test I added was I said the suppression removal was the regression guard and the unit self.assertEqual(sys.maxsize, bytesIo.seek(sys.maxsize))
self.assertEqual(self.EOF, bytesIo.read(4))That is exactly the case where the arithmetic wraps, and it is what Verified rather than argued, building with CI's own configuration
So small overseeks produce no report at all, which is why the existing @picnixz — if you would still prefer an explicitly-named regression Unrelated but worth flagging, since it cost me a while and may cost
@StanFromIreland this may be worth a line in |
Dropping the '!' from :meth:`!io.StringIO.read` while trimming the entry broke the docs build: there is no doc target for io.StringIO.read, so nitpicky mode reports 'py:meth reference target not found'. The original had the bang for exactly that reason. Every build check passed; Docs was the only real failure of the three (the other two are the unresolved-review gate and the aggregate that depends on it).
_io_StringIO_read_impl()clamps the read size to zero when the position is past the end of the buffer, but still forms the pointerself->buf + self->posbefore returning. Nothing is read through it, but forming it is undefined behaviour on its own, and sinceself->bufis aPy_UCS4 *the byte offset isself->pos * 4— so forself->pos == 2**62 - 1the multiplication wraps and yields a pointer four bytes belowself->buf._stringio_readline()already guards exactly this condition a few lines below, with the comment "In case of overseek, return the empty string", so this applies the same guard on the read path.self->pos += sizeis a no-op here becausesizeis zero, andENSURE_REALIZED()is deliberately left ahead of the guard so the state transition and its error path are unchanged.The existing
test_iosuite already covers this path — that is how UBSan found it — so no new test is needed. With theModules/_io/stringio.centry removed fromTools/ubsan/suppressions.txt,test_iopasses underUBSAN_OPTIONS=halt_on_error=1, where before it aborts atstringio.c:352.