Skip to content

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

Closed

Description

@mendlero

Bug report

Bug description:

If you run the following code, which uses fetchmany(0), you see it will act like fetchall by fetching everything instead of just returning an empty list, hypothetically, this could even be a security exploit if mismanaged.
This code was tested on both python 3.12.0 and python 3.13.7

import sqlite3

# setup
conn = sqlite3.connect(":memory:")
conn.execute('CREATE TABLE IF NOT EXISTS example_table (id INTEGER PRIMARY KEY);')
conn.execute('INSERT INTO example_table (id) VALUES (1), (2), (3);')
conn.commit()

# bug
cursor = conn.execute('SELECT id FROM example_table;')
ids = cursor.fetchmany(0)
print(ids)  # Expected: [], Actual: [(1,), (2,), (3,)]
remaining = cursor.fetchall() # will be []
print(remaining)

"""
TERMINAL EXECUTION ON WINDOWS (CMD):
>python test.py
[(1,), (2,), (3,)]
[]
"""

CPython versions tested on:

3.12, 3.13

Operating systems tested on:

Windows

Linked PRs

Activity

  1. picnixz commented on Sep 24, 2025

    @picnixz
    Member

    I agree. The issue is in the C code that checks the following:

            if (++counter == maxrows) {
                break;
            }

    however counter starts from 0, so this condition will never hold. I'll make a PR.

  2. self-assigned this
    on Sep 24, 2025
  3. picnixz commented on Sep 24, 2025

    @picnixz
    Member

    There are also other issues. Passing a negative value would also trigger the issue. For now, I'm restricting the number of items on an unsigned int, but we should eventually allow a positive ssize_t for consistency (the maximum number of items a list could theoretically contain).

  4. picnixz commented on Sep 24, 2025

    @picnixz
    Member

    Great, because of the legacy PyMemerDef API, for writable fields, protection against signed values into unsigned ones is not guaranteed.

  5. picnixz commented on Sep 24, 2025

    @picnixz
    Member

    TL;DR: fetchmany(x) for $x \leq 0$ is currently equivalent to fetchall().

    I'm hesistant to actually consider this as a security issue. It's not possible to have an infinite loop as it's limited to the number of records that can be fetched (which could still be huge). I would expect fetchmany() to be parametrized once and for all, so that records are produced in chunks and then processed separately. What could be concerning is the possible peak memory.

    On the other hand, I wonder whether it's common for a user to be able to actually change arraysize or the parameter passed to fetchmany(). I don't think it's common in production code.

    Do you concur with this analysis, @sethmlarson? I'll remove the security tag for now, but feel free to consider it a security issue due to the possible peak memory usage (but again the attack surface is IMO quite small and the impact could remain limited in practice.

  6. mendlero commented on Sep 24, 2025

    @mendlero
    Author

    @picnixz I agree this can hardly be a security vulnrability on its own, but imagine something like this:

    MESSAGE_FETCH_LIMIT = ...
    
    def fetch_messages(conn: sqlite3.Connection, count: int):
       ######### ACCEPTS COUNT=0
       if count < 0 or count > MESSAGE_FETCH_LIMIT:
            raise ValueError()
       cur = conn.execute(...)
       return cur.fetchmany(count)
       
    
    def send_messages_to_client(conn: sqlite3.Connection, message_count: int):
       messages = fetch_messages(conn, message_count)
       # loop with indexes
       for (message, i) in zip(messages, range(MESSAGE_FETCh_LIMIT):
           messages[i] = strip_secrects_from_message(message)
       
       send_to_client(conn, messages)

    The bug obviously occurs when message_count is 0 and can cause unwanted data to be sent back to the client (I had something similar in my code). It can be mitigated and patched in several ways, yet the program functions as wanted as long as fetchmany(0) returns an empty array.
    As rediculous as the above code may be, this could be used to leak unwanted information and should therfore be treated like a security vulnarability.

  7. picnixz commented on Sep 24, 2025

    @picnixz
    Member

    On the other hand, I wonder whether it's common for a user to be able to actually change arraysize or the parameter passed to fetchmany(). I don't think it's common in production code.

    Well color me surprised but fetchmany() can indeed be used for fetching a set of messages as you illustrated. Although I would never pass a bare "message_count" without a middleware validation, I agree that poor production code could be vulnerable. Note that the docs somewhat advise against this practice:

    Note there are performance considerations involved with the size parameter. For optimal performance, it is usually best to use the arraysize attribute. If the size parameter is used, then it is best for it to retain the same value from one fetchmany() call to the next.

    The second sentence indicates that it shouldn't be "parametrizable" in general.

    As rediculous as the above code may be, this could be used to leak unwanted information

    I would be careful about this. If the client wants to get say their last "0" messages, then they would just get all of them in this example. But the messages are already theirs so we should be clear about the "unwanted" information.


    For now, I'll keep the security-tag and wait for Seth's confirmation. The fact that we can actually pass a negative size (possibly by mistake) could be a concern though. The backports shouldn't be too annoying.

  8. sethmlarson commented on Sep 24, 2025

    @sethmlarson
    Contributor

    I think it's okay to handle this as a normal bug instead of as a security issue. If a value is user-controlled and being used within fetchmany() unsanitized, it's likely that an extremely large value would also be accepted. The "severity" even if this is possible is questionable, too.

  9. added a commit that references this issue on Sep 30, 2025
  10. added a commit that references this issue on Sep 30, 2025
  11. added 5 commits that reference this issue on Sep 30, 2025
  12. picnixz commented on Sep 30, 2025

    @picnixz
    Member

    Thank you for the report. I provided a fix, and am waiting for backports to be merged.

  13. added a commit that references this issue on Oct 7, 2025
  14. added a commit that references this issue on Oct 7, 2025
  15. hugovk commented on Oct 8, 2025

    @hugovk
    Member

    Backports merged to all bugfix branches.

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

Metadata

Metadata

Assignees

Labels

Projects

Milestone

No milestone

Relationships

None yet

Development

No branches or pull requests

Issue actions