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

[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

• gh-139283: correctly handle size limit in cursor.fetchmany() #139296
• [3.14] gh-139283: correctly handle size limit in cursor.fetchmany() (GH-139296) #139441
• [3.13] gh-139283: correctly handle size limit in cursor.fetchmany() (GH-139296) #139444

[picnixz]

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.

[picnixz]

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).

[picnixz]

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

[picnixz]

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.

[mendlero]

@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.

[picnixz]

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.