This issue tracker has been migrated to GitHub, and is currently read-only.
For more information, see the GitHub FAQs in the Python's Developer Guide.

classification
Title: [sqlite3] SQLITE_LIMIT_LENGTH is incorrectly used to check statement length
Type: behavior Stage: resolved
Components: Extension Modules Versions: Python 3.11
process
Status: closed Resolution: fixed
Dependencies: Superseder:
Assigned To: Nosy List: erlendaasland, pablogsal, serhiy.storchaka
Priority: normal Keywords: patch

Created on 2021-11-08 17:40 by erlendaasland, last changed 2022-04-11 14:59 by admin. This issue is now closed.

Pull Requests
URL Status Linked Edit
PR 29489 merged erlendaasland, 2021-11-09 14:50
Messages (8)
msg405975 - (view) Author: Erlend E. Aasland (erlendaasland) * (Python triager) Date: 2021-11-08 17:40
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
msg405976 - (view) Author: Erlend E. Aasland (erlendaasland) * (Python triager) Date: 2021-11-08 17:40
BTW, this only affects the code in main.
msg405979 - (view) Author: Serhiy Storchaka (serhiy.storchaka) * (Python committer) Date: 2021-11-08 18:47
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"?
msg405983 - (view) Author: Erlend E. Aasland (erlendaasland) * (Python triager) Date: 2021-11-08 20:02
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.
msg406023 - (view) Author: Erlend E. Aasland (erlendaasland) * (Python triager) Date: 2021-11-09 13:57
> > 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
msg406039 - (view) Author: Erlend E. Aasland (erlendaasland) * (Python triager) Date: 2021-11-09 19:10
Also, SQLITE_LIMIT_LENGTH and SQLITE_LIMIT_SQL_LENGTH is inclusive in SQLite. We should treat them likewise.
msg406041 - (view) Author: Erlend E. Aasland (erlendaasland) * (Python triager) Date: 2021-11-09 19:15
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.
msg406122 - (view) Author: Pablo Galindo Salgado (pablogsal) * (Python committer) Date: 2021-11-10 18:46
New changeset c1323d4b8cb010a06c11bace6e681bea7f895851 by Erlend Egeberg Aasland in branch 'main':
bpo-45754: Use correct SQLite limit when checking statement length (GH-29489)
https://github.com/python/cpython/commit/c1323d4b8cb010a06c11bace6e681bea7f895851
History
Date User Action Args
2022-04-11 14:59:52adminsetgithub: 89915
2021-11-10 21:40:29erlendaaslandsetstatus: open -> closed
resolution: fixed
stage: patch review -> resolved
2021-11-10 18:46:20pablogsalsetnosy: + pablogsal
messages: + msg406122
2021-11-09 19:15:35erlendaaslandsetmessages: + msg406041
2021-11-09 19:10:11erlendaaslandsetmessages: + msg406039
2021-11-09 14:50:31erlendaaslandsetkeywords: + patch
stage: patch review
pull_requests: + pull_request27740
2021-11-09 13:57:16erlendaaslandsetmessages: + msg406023
2021-11-08 20:02:43erlendaaslandsetmessages: + msg405983
2021-11-08 18:47:26serhiy.storchakasetmessages: + msg405979
2021-11-08 17:40:32erlendaaslandsettype: behavior
messages: + msg405976
components: + Extension Modules
versions: + Python 3.11
2021-11-08 17:40:00erlendaaslandcreate