gh-150449: Fix sqlite3.Blob crash with negative-step slices#150450
gh-150449: Fix sqlite3.Blob crash with negative-step slices#150450ever0de wants to merge 11 commits into
Conversation
ever0de
force-pushed
the
gh-150449-fix-sqlite-blob-negative-step-slice
branch
from
561ce7f to
24ade8a
Compare
| // For positive step: read the contiguous range [start, stop) and pick | ||
| // every step-th byte. For negative step: read the contiguous range | ||
| // [start+(len-1)*step, start] and pick bytes in reverse stride order. | ||
| Py_ssize_t read_offset, read_length; |
There was a problem hiding this comment.
We have an adjust slice function somewhere in the codebase, use the latter instead.
There was a problem hiding this comment.
sqlite3.Blob has no direct memory pointer — data can only be fetched via sqlite3_blob_read(blob, buf, nbytes, offset), which reads a single contiguous byte range. To keep that to one call, we must pre-compute the minimal covering range with Py_MIN/Py_ABS, then index into the local buffer as blob_buf[cur - read_offset].
The size_t cur += step loop itself is the same pattern as bytearrayobject.c; only the offset correction differs because our local buffer starts at read_offset, not 0.
Py_ssize_t start, stop, step, slicelength, i;
size_t cur;
PySlice_Unpack(index, &start, &stop, &step);
slicelength = PySlice_AdjustIndices(PyByteArray_GET_SIZE(self),
&start, &stop, step);
char *source_buf = PyByteArray_AS_STRING(self);
char *result_buf = PyByteArray_AS_STRING(result);
for (cur = start, i = 0; i < slicelength; cur += step, i++) {
result_buf[i] = source_buf[cur];
}Two approaches are possible:
-
Current approach (one read,
Py_MIN/Py_ABS): Pre-compute the minimal covering range, read it once, then index withblob_buf[cur - read_offset]. Thesize_t cur += steploop is the same as bytearrayobject.c; only the offset correction differs. -
N individual reads: Call
sqlite3_blob_readonce per element inside the loop — noPy_MIN/Py_ABSneeded, but O(n) SQLite API calls instead of one.
I went with 1 to keep the number of SQLite calls minimal, but happy to switch to 2 if you prefer simpler code over fewer API calls.
picnixz
left a comment
picnixz
left a comment
There was a problem hiding this comment.
Please also exercise negative stepsize when start <= end (it doesn't make sense but still it needs to be checked).
| } | ||
| else { | ||
| PyObject *blob_bytes = read_multiple(self, stop - start, start); | ||
| // For positive step: read the contiguous range [start, stop) and |
There was a problem hiding this comment.
It's not a contiguous range if step != 1.
|
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 |
Reading or writing a sqlite3.Blob with a negative-step slice such as blob[9:0:-2] caused a crash (SystemError on older versions, ValueError on current main) because the C code computed stop - start as the read length, which is negative for negative steps. Fix subscript_slice() and ass_subscript_slice() in Modules/_sqlite/blob.c to handle both positive and negative steps correctly: - For step > 1: keep reading [start, stop) and indexing forward as before. - For step < -1: read the contiguous range [start+(len-1)*step, start] and index backward with stride -step.
ever0de
force-pushed
the
gh-150449-fix-sqlite-blob-negative-step-slice
branch
from
24ade8a to
af0eb65
Compare
|
Please do NOT force-push; read https://devguide.python.org/getting-started/pull-request-lifecycle/ first. |
|
Sorry.. I executed the wrong command. |
When a negative-step slice has start <= stop (e.g. blob[3:8:-1]), the slice is empty (slicelength == 0). Previously ass_subscript_slice() returned 0 immediately without acquiring the value buffer, so assigning a non-empty bytes object to such a slice silently succeeded instead of raising IndexError. Move PyObject_GetBuffer() and the size check before the len == 0 early return so that the element-count mismatch is always detected.
Exercise the case where step is negative but start <= stop, which produces an empty slice. Verify that: - blob[3:8:-1] returns b"" (no crash) - blob[3:8:-1] = b"" is a no-op - blob[3:8:-1] = b"abc" raises IndexError (wrong size) Also add blob[5:5:-1] == b"" to cover the start == stop edge case.
|
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. |
serhiy-storchaka
left a comment
serhiy-storchaka
left a comment
There was a problem hiding this comment.
I think this should be classified as a new feature. Please add a versionchanged directive for blob documentation and the What's New entry.
| return -1; | ||
| } | ||
|
|
||
| if (len == 0) { |
There was a problem hiding this comment.
Was there a different bug? Like blob[5:5] = None and blob[5:5] = b'123'? We need tests for such cases.
There was a problem hiding this comment.
Yes, these are bugs
the original code silently ignores both type errors and size mismatches when the slice length is zero, while the same violations raise errors for non-empty slices:
| Operation | Result (Python 3.13.4) |
|---|---|
blob[0:3] = b'12345' |
IndexError: Blob slice assignment is wrong size |
blob[0:3] = None |
TypeError: a bytes-like object is required |
blob[5:5] = b'123' |
no error (blob unchanged) |
blob[5:5] = None |
no error |
The same invariant (correct type, matching size) should hold regardless of whether the slice is empty.
I've added tests to cover both cases.
| // update each element using the standard size_t-cursor pattern that | ||
| // handles both positive and negative steps via unsigned arithmetic. | ||
| Py_ssize_t last = start + (len - 1) * step; | ||
| Py_ssize_t read_offset = Py_MIN(start, last); |
There was a problem hiding this comment.
Should not this be a write_offset?
Classify negative-step slice support as a new feature per reviewer feedback (serhiy-storchaka): - Add versionchanged:: 3.16 directive to sqlite3.Blob documentation noting that negative-step slices are now supported. - Add What's New entry under Doc/whatsnew/3.16.rst. - Update NEWS entry wording to describe the change as a feature. - Add tests for empty-slice assignment edge cases (blob[5:5] = None and blob[5:5] = b'123') to verify TypeError and IndexError are raised correctly. - Rename read_offset to write_offset in ass_subscript_slice() since that variable is used as the offset for inner_write(), not a read.
Per devguide markup guidelines, the version argument for versionchanged should be 'next' during development; the release manager replaces it with the actual version number at release time.
…ystemError or ValueError'
…-sqlite-blob-negative-step-slice
Documentation build overview
|
| Py_ssize_t last = start + (len - 1) * step; | ||
| Py_ssize_t read_offset = Py_MIN(start, last); | ||
| Py_ssize_t write_offset = Py_MIN(start, last); | ||
| Py_ssize_t read_length = Py_ABS(start - last) + 1; |
There was a problem hiding this comment.
Should not it be write_length?
3 participants
Reading or writing a sqlite3.Blob with a negative-step slice such as blob[9:0:-2] caused a crash (SystemError on older versions, ValueError on current main) because the C code computed stop - start as the read length, which is negative for negative steps.
Fix subscript_slice() and ass_subscript_slice() in Modules/_sqlite/blob.c to handle both positive and negative steps correctly: