Skip to content

gh-150449: Fix sqlite3.Blob crash with negative-step slices - #150450

Open
ever0de wants to merge 17 commits into
python:mainfrom
ever0de:gh-150449-fix-sqlite-blob-negative-step-slice
Open

gh-150449: Fix sqlite3.Blob crash with negative-step slices#150450
ever0de wants to merge 17 commits into
python:mainfrom
ever0de:gh-150449-fix-sqlite-blob-negative-step-slice

Conversation

@ever0de

@ever0deever0de commented May 26, 2026

Copy link
Copy Markdown
Contributor

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
ever0deforce-pushed the gh-150449-fix-sqlite-blob-negative-step-slice branch from 561ce7f to 24ade8aCompareMay 26, 2026 06:58
@ever0de
ever0de marked this pull request as ready for review May 26, 2026 06:58
Comment threadModules/_sqlite/blob.c Outdated
// 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;

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We have an adjust slice function somewhere in the codebase, use the latter instead.

@ever0deever0deMay 26, 2026

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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_tstart, stop, step, slicelength, i;
size_tcur;
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:

  1. Current approach (one read, Py_MIN/Py_ABS): Pre-compute the minimal covering range, read it once, then index with blob_buf[cur - read_offset]. The size_t cur += step loop is the same as bytearrayobject.c; only the offset correction differs.

  2. N individual reads: Call sqlite3_blob_read once per element inside the loop — no Py_MIN/Py_ABS needed, 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.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I wanted to know whether you could use PySlice_AdjustIndices() but I guess not?

@picnixzpicnixz left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please also exercise negative stepsize when start <= end (it doesn't make sense but still it needs to be checked).

Comment threadModules/_sqlite/blob.c Outdated
}
else {
PyObject *blob_bytes = read_multiple(self, stop - start, start);
// For positive step: read the contiguous range [start, stop) and

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's not a contiguous range if step != 1.

@bedevere-app

Copy link
Copy Markdown

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.

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
ever0deforce-pushed the gh-150449-fix-sqlite-blob-negative-step-slice branch from 24ade8a to af0eb65CompareMay 26, 2026 08:32
@picnixz

Copy link
Copy Markdown
Member

Please do NOT force-push; read https://devguide.python.org/getting-started/pull-request-lifecycle/ first.

@ever0de

Copy link
Copy Markdown
ContributorAuthor

Sorry.. I executed the wrong command.

ever0de added 3 commits May 26, 2026 17:42
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.
@ever0de

Copy link
Copy Markdown
ContributorAuthor

I have made the requested changes; please review again

@bedevere-app

Copy link
Copy Markdown

Thanks for making the requested changes!

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

@serhiy-storchakaserhiy-storchaka left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this should be classified as a new feature. Please add a versionchanged directive for blob documentation and the What's New entry.

Comment threadModules/_sqlite/blob.c Outdated
return -1;
}

if (len == 0) {

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Was there a different bug? Like blob[5:5] = None and blob[5:5] = b'123'? We need tests for such cases.

@ever0deever0deMay 30, 2026

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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:

OperationResult (Python 3.13.4)
blob[0:3] = b'12345'IndexError: Blob slice assignment is wrong size
blob[0:3] = NoneTypeError: a bytes-like object is required
blob[5:5] = b'123'no error (blob unchanged)
blob[5:5] = Noneno 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.

Comment threadModules/_sqlite/blob.c Outdated
ever0de added 3 commits May 30, 2026 13:42
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.
@ever0de
ever0de requested a review from AA-Turner as a code ownerMay 30, 2026 05:51
@read-the-docs-community

read-the-docs-communityBot commented May 30, 2026

Copy link
Copy Markdown

@serhiy-storchakaserhiy-storchaka left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM. 👍

Comment threadModules/_sqlite/blob.c Outdated
@serhiy-storchaka

Copy link
Copy Markdown
Member

@ever0de, as this PR combines the bug fix (for several bugs) and a new feature, it may be better to split it on two -- one for the bug fix, it will be backported, and other on the top of it only for a new feature.

@ever0de

ever0de commented Jun 4, 2026

Copy link
Copy Markdown
ContributorAuthor

@serhiy-storchaka I opened issue #150913 for the empty-slice validation bug.

My plan is to submit a separate bug-fix PR for #150913 and remove the bug-fix changes from #150450, leaving #150450 focused on the negative-step slice feature.

Does that separation look good to you?


backlink: #150915

…ythongh-150913
The empty-slice assignment validation change (moving the len==0 early
return to after buffer/type/size checks) has been split out to a
separate bug-fix PR targeting issue python#150913.
This branch retains only the negative-step slice feature.
@serhiy-storchaka

Copy link
Copy Markdown
Member

Yes, that's it. Thank you.

@picnixzpicnixz left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would like you to either 1) explain why we need the casts 2) or remove them

Comment on lines +468 to +471
size_t cur;
Py_ssize_t i;
for (cur = (size_t)start, i = 0; i < len; cur += (size_t)step, i++) {
res_buf[i] = blob_buf[(Py_ssize_t)cur - read_offset];

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

OOC why all those casts? start and atep are already ssize values, and then we recast cur to ssize_t. Why not having ssize_t everywhere? there should not be any issues with signed comparisons as both operands are signed.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

SSIZE_MAX + SSIZE_MAX = integer overflow.

The code is more complicated with signed integers.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh right. If we can add a test case for that it would be good but if it is too complex to do it (by mocking some stuff), let's forget it. I am keeping the comment unresolved so that future readers will remember

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Would it be worth adding an inline comment here to explain why size_t is used instead of Py_ssize_t? Something like:

// Use size_t for 'cur' so that negative steps work via well-defined// unsigned wrap-around arithmetic. A signed accumulator would risk// undefined behaviour when start or step is near SSIZE_MAX.

@serhiy-storchakaserhiy-storchakaJun 5, 2026

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No need. We use this technique in other places too. And this is not about negative steps, you will never get integer overflow in cur += step for negative step. But you get it for large positive step.

So please add a test case for something like b[5::sys.maxsize]. You can also add a test case for step=-sys.maxsize-1.

Comment threadModules/_sqlite/blob.c
ever0deand others added 4 commits June 5, 2026 16:58
…ice operations
Add test cases for sys.maxsize and -sys.maxsize-1 step values in both
read (test_blob_get_slice_with_negative_step) and write
(test_blob_set_slice_with_negative_step) to cover the unsigned wrap-around
arithmetic path that avoids signed integer overflow.
…in sqlite3.Blob
Extend the existing extreme-step test cases (sys.maxsize / -sys.maxsize-1)
in test_blob_set_slice_with_negative_step to verify the blob contents
were actually written correctly, not just that no crash occurred.
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants

@ever0de@picnixz@serhiy-storchaka