[sqlite3] SQLITE_LIMIT_LENGTH is incorrectly used to check statement length

[erlend-aasland]

BPO	45754
Nosy	@serhiy-storchaka, @pablogsal, @erlend-aasland
PRs	bpo-45754: Use correct SQLite limit when checking statement length #29489

^{Note: these values reflect the state of the issue at the time it was migrated and might not reflect the current state.}

Show more details

GitHub fields:

assignee = None
closed_at = <Date 2021-11-10.21:40:29.743>
created_at = <Date 2021-11-08.17:40:00.947>
labels = ['extension-modules', 'type-bug', '3.11']
title = '[sqlite3] SQLITE_LIMIT_LENGTH is incorrectly used to check statement length'
updated_at = <Date 2021-11-10.21:40:29.742>
user = 'https://github.com/erlend-aasland'

bugs.python.org fields:

activity = <Date 2021-11-10.21:40:29.742>
actor = 'erlendaasland'
assignee = 'none'
closed = True
closed_date = <Date 2021-11-10.21:40:29.743>
closer = 'erlendaasland'
components = ['Extension Modules']
creation = <Date 2021-11-08.17:40:00.947>
creator = 'erlendaasland'
dependencies = []
files = []
hgrepos = []
issue_num = 45754
keywords = ['patch']
message_count = 8.0
messages = ['405975', '405976', '405979', '405983', '406023', '406039', '406041', '406122']
nosy_count = 3.0
nosy_names = ['serhiy.storchaka', 'pablogsal', 'erlendaasland']
pr_nums = ['29489']
priority = 'normal'
resolution = 'fixed'
stage = 'resolved'
status = 'closed'
superseder = None
type = 'behavior'
url = 'https://bugs.python.org/issue45754'
versions = ['Python 3.11']

[erlend-aasland]

In Modules/_sqlite/statement.c pysqlite_statement_create() and Modules/_sqlite/cursor.c pysqlite_cursor_executescript_impl(), we incorrectly use SQLITE_LIMIT_LENGTH to check statement length. However, the correct limit is *SQLITE_LIMIT_SQL_LENGTH*.

### Alternative 1:
Quick fix is to check against SQLITE_LIMIT_SQL_LENGTH instead of SQLITE_LIMIT_LENGTH.

### Alternative 2:
Let SQLite do the check for us, and instead add integer overflow check, since Py_ssize_t may be larger than int (sqlite3_prepare_v2() uses an int as the max statement length parameter).

### Alternative 3:
As alternative 2, but alter the sqlite3_prepare_v2() call to accept _any_ length (max statement length = -1).

See also:

• https://sqlite.org/limits.html
• https://sqlite.org/c3ref/c_limit_attached.html
• https://sqlite.org/c3ref/prepare.html

[erlend-aasland]

BTW, this only affects the code in main.

[serhiy-storchaka]

In alternative 1 we control the type and the message of exception. In alternative 3 we left this to the SQLite3 engine. It is difficult to keep consistency in alternative 2, errors raised for sizes less and larger than INT_MAX can be different. I think this is a serious drawback of this alternative.

What happens if set SQLITE_LIMIT_SQL_LENGTH to 0 or negative value? Would it be interpreted as "no limit"?

[erlend-aasland]

I'm leaning towards alt 1. It is also easy to test all paths with that option. But I also think alt 3 is ok. It makes for easier code to maintain.

Alt 2 is, as you say, the weakest alternative.

What happens if set SQLITE_LIMIT_SQL_LENGTH to 0 or negative value? Would it be interpreted as "no limit"?

I'm not sure about 0; I'll try to find out.

If you supply -1, you are querying the limit category, not setting it.

[erlend-aasland]

> What happens if set SQLITE_LIMIT_SQL_LENGTH to 0 [...]

In this case, the limit is actually zero:

    >>> cx.setlimit(sqlite3.SQLITE_LIMIT_SQL_LENGTH, 0)
    1024
    >>> cx.execute("select 1")
    Traceback (most recent call last):
      File "<stdin>", line 1, in <module>
    sqlite3.DataError: string or blob too big

[erlend-aasland]

Also, SQLITE_LIMIT_LENGTH and SQLITE_LIMIT_SQL_LENGTH is inclusive in SQLite. We should treat them likewise.

[erlend-aasland]

I believe it is best to go with alternative 1. The error strings produced by SQLite may change from release to release, so for example a buildbot update may suddenly break the CI.

[pablogsal]

New changeset c1323d4 by Erlend Egeberg Aasland in branch 'main':
bpo-45754: Use correct SQLite limit when checking statement length (GH-29489)
c1323d4