GeezerVison

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 I have made the requested changes; please review again. I will then notify any core developers who have left a review that you're ready for them to take another look at this pull request.

Thanks both.

Regression test — added in 0d659fa, as test_overseek_far_past_end.

One thing worth being explicit about, because it affects how much the
test is worth: it passes on unfixed builds too. I checked before
writing it — io.StringIO and _pyio.StringIO, and BytesIO for both,
already return '' with the correct tell() and getvalue() at every
offset it tries. The bug is undefined behaviour, and UB is not generally
observable; a normal build gives no signal here.

So the test pins the documented behaviour rather than detecting the
defect. It is still worth having: the existing test_overseek only
seeks one past the end, so nothing covered the large-offset case at all,
and that is precisely where the arithmetic wraps — the pointer overflow
only occurs above PY_SSIZE_T_MAX // 4. It is in MemoryTestMixin, so
it runs for BytesIO as well as StringIO, C and pure-Python.

What actually guards the fix is the other half of the diff: removing

pointer-overflow:Modules/_io/stringio.c

from Tools/ubsan/suppressions.txt, which .github/workflows/reusable-san.yml
consumes. With the suppression gone, the UBSan job fails if the UB comes
back. That is the regression guard; the unit test is the behavioural
one. Happy to add more of the latter if you would like, but I did not
want to imply it catches what it does not.

NEWS entry — trimmed to the user-visible statement, no macro names,
no description of the fix, no suppressions note.

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.

Correction to my last comment, and a retraction: the test I added was
redundant and I have removed it (9d3ba83).

I said the suppression removal was the regression guard and the unit
test the behavioural one. The first half is right; the second oversold
it. CStringIOTest.testSeek already covers this — it has all along:

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
produced the diagnostic recorded in Tools/ubsan/suppressions.txt.

Verified rather than argued, building with CI's own configuration
(--with-undefined-behavior-sanitizer --with-strict-overflow, clang 22):

So small overseeks produce no report at all, which is why the existing
test_overseek never surfaced this, and why my large-offset test added
no coverage that testSeek did not already provide. I have left a
comment on those two lines instead — the test's name gives no hint that
they guard a UB fix, and trimming them would silently remove the
coverage.

@picnixz — if you would still prefer an explicitly-named regression
test even though it duplicates existing coverage, say so and I will add
one back; I did not want to leave a test in that could not fail.

Unrelated but worth flagging, since it cost me a while and may cost
others the same. -fno-strict-overflow silently disables clang's
pointer-overflow sanitizer, and re-adding -fsanitize=pointer-overflow
afterwards does not bring it back:

-fsanitize=undefined                                          -> reports
-fsanitize=undefined -fno-strict-overflow                     -> silent
-fsanitize=undefined -fno-strict-overflow
                     -fsanitize=pointer-overflow              -> silent

--with-strict-overflow defaults to no, so an ordinary
./configure --with-undefined-behavior-sanitizer build cannot see any
pointer-overflow: finding. The CI job is fine — reusable-san.yml
passes --with-strict-overflow explicitly — but anyone reproducing a
suppressions-file entry locally with the obvious configure line will get
silence and reasonably conclude the entry is stale. I did, for several
builds, before spotting it.

@StanFromIreland this may be worth a line in Tools/ubsan/README or
next to the pointer-overflow entries, given you own that file. Happy
to send that as a separate PR if useful.

@picnixz Please see the new version whenever you have time.

Thanks, and sorry for the back-and-forth — let me make the case properly,
because there is a close precedent that cuts both ways and I'd rather
address it head-on.

The identical bug in BytesIO was fixed as gh-141311 (#141333): forming
buf + pos with pos past the end of the buffer. That PR added a named
regression test (test_issue141311), and I expect that's the shape you
have in mind here. The difference is what a test can observe: gh-141311
was an assertion failure, so a plain Python test goes red on an unfixed
build. This bug is only observable to UBSan — I added exactly that test
(0d659fa, test_overseek_far_past_end) and it passes on unfixed
builds, because the post-fix behaviour is byte-for-byte the pre-fix
behaviour: both paths return the empty-string singleton
(PyUnicode_FromKindAndData(kind, …, 0) also funnels into
_Py_RETURN_UNICODE_EMPTY()), with the same tell() afterwards. A test
that cannot fail seemed worse than none, so I removed it (9d3ba83)
rather than leave something that looks like a guard but isn't.

The actual regression guard is the Tools/ubsan/suppressions.txt entry
removal: since #148168 UBSan hard-fails CI on new reports, and the
existing CStringIOTest.testSeek —

self.assertEqual(sys.maxsize, bytesIo.seek(sys.maxsize))
self.assertEqual(self.EOF, bytesIo.read(4))

— is precisely the case where pos * sizeof(Py_UCS4) wraps; I verified
on an unfixed build (--with-undefined-behavior-sanitizer --with-strict-overflow, CI's own configuration) that those two lines are
what produced the diagnostic recorded in the suppressions file. So if
anyone reintroduces the unguarded arithmetic, the existing suite fails
under the UBSan job with no new test needed. I added a comment on those
lines saying so, since the test's name gives no hint.

If you'd still rather have an explicitly named
test_overseek_far_past_end even knowing it can't fail pre-fix, I'm
happy to add it back — it does document intent. Just say the word.

I have made the requested changes; please review again.

Added test_read_far_past_end (C and Python StringIO). I want to be straight about what it does and does not do, since I retracted an earlier attempt in this thread: on a normal build it cannot fail on the unfixed code, because the bug is pointer UB with no observable Python-level effect (I checked: unfixed, read() already returns '' with tell() unchanged at every offset it tries). It pins the documented behaviour at the large offsets nothing else covered, and under a UBSan build — now that this PR removes the Modules/_io/stringio.c entry from Tools/ubsan/suppressions.txt — it is the reproducer that reports at stringio.c. This mirrors gh-141311, where the BytesIO overseek fix landed with test_issue141311, a behavioural EOF test of the same non-observable-on-release kind.

Happy to drop it and rely on the existing testSeek (which already seeks to sys.maxsize and reads) plus the suppression removal if you'd prefer the suite not carry a near-duplicate.

Thanks for making the requested changes!

@picnixz: please review the changes made to this pull request.

This PR is stale because it has been open for 90 days with no activity.