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 4 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. |
2 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: