Skip to content

sqlite3: fix Blob.__setitem__ value range validation#7981

Open
ever0de wants to merge 4 commits into
RustPython:mainfrom
ever0de:fix/sqlite-blob-set-item-error-validation
Open

sqlite3: fix Blob.__setitem__ value range validation#7981
ever0de wants to merge 4 commits into
RustPython:mainfrom
ever0de:fix/sqlite-blob-set-item-error-validation

Conversation

@ever0de
Copy link
Copy Markdown
Contributor

@ever0de ever0de commented May 26, 2026

Previously, assigning an out-of-range integer (negative or > 255) or an integer too large for i64 (e.g. 2**65) to a Blob index raised OverflowError instead of ValueError.

Mirror CPython's ass_subscript_index logic:

  • Convert the value via to_i64(), treating any overflow as -1
  • Validate the result is in [0, 255], raising ValueError("byte must be in range(0, 256)") otherwise
  • Separate deletion error messages: "item deletion" for index, "slice deletion" for slice

Ref: https://github.com/python/cpython/blob/main/Modules/_sqlite/blob.c


Problem

When assigning an out-of-range integer to a Blob index, RustPython raised OverflowError instead of ValueError:

blob[0] = -1      # OverflowError: Python int too large to convert to Rust u8
blob[0] = 256     # OverflowError
blob[0] = 2**65   # OverflowError

CPython raises ValueError: byte must be in range(0, 256) for all three cases.

Root Cause

The previous implementation used try_to_primitive::<u8>(vm)? to convert the assigned value, which calls Rust's type conversion and raises a generic OverflowError for any value that doesn't fit in u8 — including negative numbers.

Fix

Mirrors CPython's ass_subscript_index logic:

// CPython blob.c
long val = PyLong_AsLong(value);
if (val == -1 && PyErr_Occurred()) {
    PyErr_Clear();
    val = -1;  // treat overflow as out-of-range
}
if (val < 0 || val > 255) {
    PyErr_SetString(PyExc_ValueError, "byte must be in range(0, 256)");
    return -1;
}

In Rust:

  • Replace try_to_primitive::<u8>() with as_bigint().to_i64().unwrap_or(-1)
  • Values that overflow i64 (e.g. 2**65) are treated as -1, which then fails the range check
  • Raises ValueError("byte must be in range(0, 256)") for any value outside [0, 255]

Additionally, aligned the deletion error messages with CPython:

  • del blob[0]"Blob doesn't support item deletion"
  • del blob[5:10]"Blob doesn't support slice deletion"

Changes

  • _sqlite3.rs: Fix Blob::ass_subscript index branch
  • test_dbapi.py: Remove @unittest.expectedFailure from test_blob_set_item_error

Summary by CodeRabbit

  • Bug Fixes

    • Fixed single-byte BLOB assignment to properly validate byte values and handle integer overflow.
    • Improved BLOB slice assignment so unaffected bytes are preserved for complex slices (including negative-step writes).
  • Tests

    • Added a regression test for extended-slice BLOB assignments to prevent future regressions.

Review Change Stack

@coderabbitai
Copy link
Copy Markdown
Contributor

coderabbitai Bot commented May 26, 2026

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: Path: .coderabbit.yml

Review profile: CHILL

Plan: Pro

Run ID: 4f8a0be5-80a6-42b7-ade3-4e895509a59b

📥 Commits

Reviewing files that changed from the base of the PR and between 1df29c5 and c074556.

📒 Files selected for processing (1)
  • extra_tests/snippets/stdlib_sqlite.py

📝 Walkthrough

Walkthrough

This PR tightens BLOB setitem: single-index assignments now require a PyInt converted to i64 and must be 0–255; non-unit-step extended-slice assignments preserve unaffected bytes by reading the destination span first. A RustPython-only regression test verifies negative-step slice behavior.

Changes

BLOB item assignment improvements

Layer / File(s) Summary
Single-index assignment with validation
crates/stdlib/src/_sqlite3.rs
Import ToPrimitive and refactor single-index BLOB assignment to require a PyInt, convert to i64 (overflow → -1), enforce 0..=255, and write with write_single.
Extended-slice assignment with non-unit steps
crates/stdlib/src/_sqlite3.rs
Rewrite non-step == 1 slice assignment to preserve unaffected bytes: read the destination span into a temporary buffer, place incoming bytes using SaturatedSliceIter indices, and write the full span back.
BLOB negative-step slice assignment test
extra_tests/snippets/stdlib_sqlite.py
Add RustPython-only regression test verifying negative-step extended-slice assignment (blob[9:0:-2] = b"12345") produces the expected stored BLOB content.

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~25 minutes

Poem

🐰 A blob that once was hard to slice,
Now takes each byte with careful spice,
Negative steps, no bytes misplaced—
The buffer read, then bytes are graced!
From −1 to 255, all values shine,
Our BLOB assignment? Now it's fine! 🎉

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%. Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (4 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The PR title clearly and accurately summarizes the main change: fixing Blob.setitem value range validation to match CPython's behavior.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

@github-actions
Copy link
Copy Markdown
Contributor

github-actions Bot commented May 26, 2026

📦 Library Dependencies

The following Lib/ modules were modified. Here are their dependencies:

[x] lib: cpython/Lib/sqlite3
[ ] test: cpython/Lib/test/test_sqlite3 (TODO: 75)

dependencies:

  • sqlite3

dependent tests: (2 tests)

  • sqlite3: test_dbm_sqlite3 test_sqlite3

Legend:

  • [+] path exists in CPython
  • [x] up-to-date, [ ] outdated

ever0de added 2 commits May 26, 2026 11:27
Previously, assigning an out-of-range integer (negative or > 255) or an
integer too large for i64 (e.g. 2**65) to a Blob index raised OverflowError
instead of ValueError.

Mirror CPython's ass_subscript_index logic:
- Convert the value via to_i64(), treating any overflow as -1
- Validate the result is in [0, 255], raising ValueError("byte must be in range(0, 256)") otherwise
- Separate deletion error messages: "item deletion" for index, "slice deletion" for slice
In the step != 1 branch of Blob.ass_subscript, the loop used
  i_in_temp += step as usize
where step is isize. For negative steps (e.g. step = -2),
  (-2isize) as usize = 18446744073709551614
causing an out-of-bounds panic whenever slice_len >= 2.

Fix: use SaturatedSliceIter (already used by the read path) to iterate
over the correct absolute blob indices, then map each index back to a
temp buffer offset via abs_idx - range_start.

Also fix a Clippy lint: replace
  val < 0 || val > 255
with the idiomatic
  !(0..=255).contains(&val)

Add a regression test in extra_tests/snippets/stdlib_sqlite.py that
exercises blob[9:0:-2] (negative step, slice_len=5).
@ever0de ever0de force-pushed the fix/sqlite-blob-set-item-error-validation branch from ac57ee6 to 6f3eae8 Compare May 26, 2026 02:27
@ever0de ever0de marked this pull request as ready for review May 26, 2026 02:28
@ever0de
Copy link
Copy Markdown
Contributor Author

ever0de commented May 26, 2026

https://github.com/RustPython/RustPython/actions/runs/26428784978/job/77797608378?pr=7981

Why the snippet test is guarded by sys.implementation.name == "rustpython"

CI was failing because CPython 3.14 also has a bug with negative-step Blob slice assignment.

Running blob[9:0:-2] = b"12345" under CPython 3.14 raises:

SystemError: Negative size passed to PyBytes_FromStringAndSize

The root cause is in CPython's Modules/_sqlite/blob.c, inside ass_subscript_slice:

https://github.com/python/cpython/blob/b87c99158c429219fae2d6a6c49076fea68a5cad/Modules/_sqlite/blob.c#L552

// For blob[9:0:-2], PySlice_AdjustIndices gives start=9, stop=0, step=-2
PyObject *blob_bytes = read_multiple(self, stop - start, start);
//                                         0  -  9  = -9  ← negative!
// PyBytes_FromStringAndSize(NULL, -9) → SystemError

When step is negative, stop < start, so stop - start is negative, and passing that to PyBytes_FromStringAndSize triggers an internal assertion error.

This is a CPython-side bug — both CPython and RustPython had a broken implementation of negative-step Blob slice writes (CPython raises SystemError, RustPython previously panicked). This PR fixes the RustPython side. The snippet test validates RustPython's fix but cannot run on CPython until the upstream bug is resolved, hence the guard.

Reference

  1. https://docs.python.org/3/library/sqlite3.html#sqlite3.Blob
    Use indices and slices for direct access to the blob data.
  2. https://docs.python.org/3/library/stdtypes.html#typesseq-common
    s[i:j:k] — The slice of s from i to j with step k is defined as the sequence of items with index x = i + n*k such that 0 <= n < (j-i)/k. When k is negative, i and j are reduced to len(s) - 1 if they are greater.
  3. (Similar, but not the same.) assertion failure at Modules/_sqlite/blob.c:146: PyObject *read_multiple(pysqlite_Blob *, Py_ssize_t, Py_ssize_t): Assertion 'offset < sqlite3_blob_bytes(self->blob)' failed python/cpython#142787
  4. sqlite3: Blob crashes when using a negative-step slice python/cpython#150449

# this test only runs on RustPython where the fix is being validated.
import sys

if sys.implementation.name == "rustpython":
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 usually don't add rustpython-only code. is this CPython bug? Then is this reported to CPython?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Yes, the negative-step slice crash is a CPython bug — I've filed it upstream as python/cpython#150449 and submitted a fix PR (python/cpython#150450).

However, this PR also contains a few pre-existing RustPython-only bugs unrelated to that CPython issue:

  • blob[i] = v was using try_to_primitive::<u8> which silently gave the wrong error for out-of-range values (e.g. 256 or -1), instead of CPython's ValueError: byte must be in range(0, 256)
  • The item-deletion vs. slice-deletion error messages were swapped

Should I split this into two PRs — one for the CPython-mirrored negative-step fix (which could be reverted or marked TODO: RUSTPYTHON until upstream lands), and one for the RustPython-specific behavior alignment?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants