gh-139283: correctly handle `size` limit in `cursor.fetchmany()` #139296

picnixz · 2025-09-24T12:03:06Z

Using the legacy PyMemerDef API is problematic for writable fields:

If the field is tagged as Py_T_UINT or any other unsigned type, using obj.attr = -1 does NOT raise a ValueError for legacy purposes (part of the very large commit 89f507f).
Instead, I'm using plain getter/setters functions so that the "batch" size for each fetchmany() is stored as an uint32_t instead. Previously it was stored as an int.
To prevent possible counting overflows, I'm restricting maxrows in fetchmany() to be a uint32_t which I can decrement instead of having an additional counter.
I don't think we'll ever need more than 2^32 rows per fetchmany() call.

Issue: sqlite3.Cursor.fetchmany(0) fetches all values instead of none #139283

📚 Documentation preview 📚: https://cpython-previews--139296.org.readthedocs.build/

ZeroIntensity

If the issue is that Py_T_UINT doesn't do the proper validation, I'd imagine that this is an issue in other places too. Have you considered adding a private _Py_T_UINT32 that does do the validation, so we can use it elsewhere?

ZeroIntensity · 2025-09-29T12:57:25Z

Modules/_sqlite/cursor.c

 }

+/*[clinic input]
+@critical_section


A critical section won't help us much, because nothing else in Cursor uses a critical section at the moment. All other readers will race with the setter function. We also shouldn't use a critical section here -- a relaxed atomic for uint32 will be faster, but let's do that in a different PR where we fix any thread-safety issues of _sqlite entirely.

We also shouldn't use a critical section here -- a relaxed atomic for uint32 will be faster

Well, it was faster to write the critical section. I'll remove the critical sections from this PR for now if you want to address other thread safeties separately.

picnixz · 2025-09-29T13:00:34Z

If the issue is that Py_T_UINT doesn't do the proper validation, I'd imagine that this is an issue in other places too. Have you considered adding a private _Py_T_UINT32 that does do the validation, so we can use it elsewhere?

There's an open issue for that and the API is not decided (see capi-workgroup/decisions#63). So I won't bother for that yet.

ZeroIntensity · 2025-09-29T13:20:59Z

There's an open issue for that and the API is not decided (see capi-workgroup/decisions#63). So I won't bother for that yet.

The C API WG decides on public C API. We can add private APIs all we want. If we add _Py_T_UINT32 everywhere and then the WG decides they want to make that public, it's pretty easy to switch it.

picnixz · 2025-09-29T13:25:15Z

The problem is that even with a private API, it would become too annoying in the end. I don't want to create a new private API that could later be changed to accomodate the public one. In particular, I don't want to design something that would be eventually changed. See #117031 for an initial work as well. So here, I really want to focus on fixing fetchmany() and I don't want to introduce a separate API in this PR.

picnixz · 2025-09-29T13:29:00Z

@ZeroIntensity Do you have other comments on this specific fix apart the extra API that I will not add here? (even a new private API will need tests and other things that are too much for this PR in particular, however considering the next release is soon, I want this to be fixed ASAP).

ZeroIntensity

No, this seems fine otherwise. I really think we should look into adding a private API for this afterward, especially if this problem is present in other places.

miss-islington-app · 2025-09-30T09:18:59Z

Thanks @picnixz for the PR 🌮🎉.. I'm working now to backport this PR to: 3.13, 3.14.
🐍🍒⛏🤖

pythonGH-139296) Passing a negative or zero size to `cursor.fetchmany()` made it fetch all rows instead of none. While this could be considered a security vulnerability, it was decided to treat this issue as a regular bug as passing a non-sanitized *size* value in the first place is not recommended. (cherry picked from commit bc172ee) Co-authored-by: Bénédikt Tran <[email protected]>

miss-islington-app · 2025-09-30T09:19:20Z

Sorry, @picnixz, I could not cleanly backport this to 3.13 due to a conflict.
Please backport using cherry_picker on command line.

cherry_picker bc172ee8307431caf4c89612e9e454081635191f 3.13

bedevere-app · 2025-09-30T09:19:24Z

GH-139441 is a backport of this pull request to the 3.14 branch.

…hmany()` (pythonGH-139296) Passing a negative or zero size to `cursor.fetchmany()` made it fetch all rows instead of none. While this could be considered a security vulnerability, it was decided to treat this issue as a regular bug as passing a non-sanitized *size* value in the first place is not recommended. (cherry picked from commit bc172ee) Co-authored-by: Bénédikt Tran <[email protected]>

bedevere-app · 2025-09-30T09:27:48Z

GH-139444 is a backport of this pull request to the 3.13 branch.

…hmany()` (pythonGH-139296) Passing a negative or zero size to `cursor.fetchmany()` made it fetch all rows instead of none. While this could be considered a security vulnerability, it was decided to treat this issue as a regular bug as passing a non-sanitized *size* value in the first place is not recommended. (cherry picked from commit bc172ee) Co-authored-by: Bénédikt Tran <[email protected]>

bedevere-app · 2025-09-30T09:32:19Z

GH-139444 is a backport of this pull request to the 3.13 branch.

bedevere-app · 2025-09-30T09:32:29Z

GH-139444 is a backport of this pull request to the 3.13 branch.

…hmany()` (pythonGH-139296) Passing a negative or zero size to `cursor.fetchmany()` made it fetch all rows instead of none. While this could be considered a security vulnerability, it was decided to treat this issue as a regular bug as passing a non-sanitized *size* value in the first place is not recommended. (cherry picked from commit bc172ee) Co-authored-by: Bénédikt Tran <[email protected]>

…)` (GH-139296) (#139444) Passing a negative or zero size to `cursor.fetchmany()` made it fetch all rows instead of none. While this could be considered a security vulnerability, it was decided to treat this issue as a regular bug as passing a non-sanitized *size* value in the first place is not recommended. (cherry picked from commit bc172ee)

…)` (GH-139296) (GH-139441) Passing a negative or zero size to `cursor.fetchmany()` made it fetch all rows instead of none. While this could be considered a security vulnerability, it was decided to treat this issue as a regular bug as passing a non-sanitized *size* value in the first place is not recommended. (cherry picked from commit bc172ee) Co-authored-by: Bénédikt Tran <[email protected]> Co-authored-by: Petr Viktorin <[email protected]>

picnixz requested review from berkerpeksag and erlend-aasland as code owners September 24, 2025 12:03

bedevere-app bot added the awaiting core review label Sep 24, 2025

bedevere-app bot mentioned this pull request Sep 24, 2025

sqlite3.Cursor.fetchmany(0) fetches all values instead of none #139283

Closed

picnixz added needs backport to 3.13 bugs and security fixes needs backport to 3.14 bugs and security fixes awaiting core review and removed awaiting core review labels Sep 24, 2025

correctly handle size limit in cursor.fetchmany()

be39d9b

picnixz force-pushed the fix/sqlite3/fetchmany-139283 branch from b86282b to be39d9b Compare September 24, 2025 12:31

picnixz added 2 commits September 24, 2025 16:51

add critical sections

00bb4c1

fix typo

2b516fa

picnixz requested a review from ZeroIntensity September 29, 2025 10:00

ZeroIntensity reviewed Sep 29, 2025

View reviewed changes

drop thread-safe patterns for now

248400d

picnixz force-pushed the fix/sqlite3/fetchmany-139283 branch from 7bb5b7a to 248400d Compare September 29, 2025 13:02

picnixz requested a review from ZeroIntensity September 29, 2025 13:03

ZeroIntensity approved these changes Sep 29, 2025

View reviewed changes

bedevere-app bot added awaiting merge and removed awaiting core review labels Sep 29, 2025

picnixz merged commit bc172ee into python:main Sep 30, 2025
49 checks passed

picnixz deleted the fix/sqlite3/fetchmany-139283 branch September 30, 2025 09:18

bedevere-app bot removed the awaiting merge label Sep 30, 2025

miss-islington-app bot assigned picnixz Sep 30, 2025

bedevere-app bot removed the needs backport to 3.14 bugs and security fixes label Sep 30, 2025

bedevere-app bot removed the needs backport to 3.13 bugs and security fixes label Sep 30, 2025

Uh oh!

gh-139283: correctly handle size limit in cursor.fetchmany() #139296

gh-139283: correctly handle size limit in cursor.fetchmany() #139296

Uh oh!

Conversation

picnixz commented Sep 24, 2025 • edited by github-actions bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ZeroIntensity left a comment

Choose a reason for hiding this comment

Uh oh!

ZeroIntensity Sep 29, 2025

Choose a reason for hiding this comment

Uh oh!

picnixz Sep 29, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

picnixz commented Sep 29, 2025

Uh oh!

ZeroIntensity commented Sep 29, 2025

Uh oh!

picnixz commented Sep 29, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

picnixz commented Sep 29, 2025

Uh oh!

ZeroIntensity left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

miss-islington-app bot commented Sep 30, 2025

Uh oh!

miss-islington-app bot commented Sep 30, 2025

Uh oh!

bedevere-app bot commented Sep 30, 2025

Uh oh!

bedevere-app bot commented Sep 30, 2025

Uh oh!

bedevere-app bot commented Sep 30, 2025

Uh oh!

bedevere-app bot commented Sep 30, 2025

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

gh-139283: correctly handle `size` limit in `cursor.fetchmany()` #139296

gh-139283: correctly handle `size` limit in `cursor.fetchmany()` #139296

picnixz commented Sep 24, 2025 •

edited by github-actions bot

Loading

picnixz Sep 29, 2025 •

edited

Loading

picnixz commented Sep 29, 2025 •

edited

Loading