gh-108021: In sqlite3, use 64-bit APIs for binding and returning text and blobs #108075

erlend-aasland · 2023-08-17T08:18:14Z

Use sqlite3_bind_blob64() iso. sqlite3_bind_blob()
Use sqlite3_result_blob64() iso. sqlite3_result_blob()
Use sqlite3_bind_text64() iso. sqlite3_bind_text()
Use sqlite3_result_text64() iso. sqlite3_result_text()

Issue: sqlite3: use 64-bit APIs where possible #108021

erlend-aasland · 2023-08-17T08:19:45Z

@serhiy-storchaka: I did not add or adjust existing tests yet.

Existing tests:

test_func_return_too_large_text, test_func_return_too_large_blob, test_too_large_string, and test_too_large_blob currently require 64-bit platform and check for 32-bit sizes.

erlend-aasland · 2023-08-17T08:34:59Z

@serhiy-storchaka: I adjusted the tests in d669736; I'm not sure if we have buildbots that can test this, though.

Doc/whatsnew/3.13.rst

serhiy-storchaka · 2023-08-17T08:55:12Z

Modules/_sqlite/connection.c

+#if SIZEOF_SIZE_T > 8
+        if (sz > 0xffffffffffffffff) {
            PyErr_SetString(PyExc_OverflowError,
-                            "string is longer than INT_MAX bytes");
+                            "Error setting result value: "
+                            "string is longer than 2**64-1 bytes");
            return -1;
        }
-        sqlite3_result_text(context, str, (int)sz, SQLITE_TRANSIENT);
+#endif


The problem with this code is that it will be excluded from compilation on all current platforms where Python can be build and on all future platforms for long time. Therefore this code will degrade.

I do not know how better to solve this issue. Try to introduce a macro PYSQLITE_MAX_SIZE for unsigned 0xffffffffffffffffu, remove conditional compilation and cast sz to unsigned size_t before comparison. Check if any compiler emit a warning about always true condition. Now somebody at least will be able to test the code by temporary reducing the limit. At least we can be sure that the code is still syntactically correct.

Hm, the SQLite API consistently uses unsigned uint_64 values, and the Python C APIs (involved here) consistently used signed ssize_t values.

if ssize_t is less than 8 bytes, we're ok; it will fit in uint_64 anyway

if ssize_t is 8 bytes or larger, we must check if the value is larger than 2**63-1.

See also the code in _pysqlite_long_as_int64 (Modules/_sqlite/util.c).

I wrote it. 😉

Anyway, it exclude from compilation simpler code.

What I propose here, to write either:

if ((size_t)sz > PYSQLITE_MAX_SIZE) {

where PYSQLITE_MAX_SIZE is defined as 0x7fffffffu (the current limit) or 0x7fffffffffffffffu (theoretical maximum), or

if ((sqlite3_uint64)sz != (size_t)sz) {

which just test the safety of integer conversion.

Thanks; I didn't have time to look at this since yesterday. Maybe I'll find time to return to it later today. Thanks for chiming in; these C data type issues are hard to get straight!

I changed it to your former suggestion.

serhiy-storchaka · 2023-08-17T09:05:52Z

Lib/test/test_sqlite3/test_types.py

-    @unittest.skipUnless(sys.maxsize > 2**32, 'requires 64bit platform')
-    @support.bigmemtest(size=2**31, memuse=4, dry_run=False)
+    @unittest.skipUnless(sys.maxsize > 2**64, 'requires (s)size_t > 64bit')
+    @support.bigmemtest(size=2**64+1, memuse=4, dry_run=False)


No, it is not the purpose of these tests. It does not test that the code raises an expected exception. It tests that the code does not corrupt the data, does not raise unexpected exception and does not crash. What would happen if remove the overflow check.

How do existing tests fail with these changes applied? Do they stop raising DataError? It is good, we should change tests for this.

It does not test that the code raises an expected exception.

The test executes assertRaises; it does test that an exception is raised. Yes, that also implies that no corruption has happened and that sqlite3 did not segfault.

The current test code does this:

assert that DataError is raised when binding very large strings or buffers

select rows from the test table and assert that no row was inserted

IOW, it verifies that sqlite3 raised an exception for large input, and that no rows were added.

Well, now the behavior is changed, it was not unintentional, and the tests should be changed as well.

[...] the tests should be changed as well.

That is part of the aim of this PR.

What happens if left these tests as is?

Well, on my machine, they actually succeed (though I have to remove the bigmem decorators in order to force them to run), so I guess they are already broken.

serhiy-storchaka · 2023-08-18T07:47:38Z

Doc/whatsnew/3.13.rst

+sqlite3
+-------
+
+* :mod:`sqlite3` now supports strings and BLOB sizes up to ``2**64-1`` bytes


Unfortunately, the hardcoded limit is still 2**31-1, according to the sources. So this change does not change much.

Yes, there are both compile time and runtime limits that are in effect. Perhaps we can just consider this an implementation detail and remove the NEWS/What's New entries.

I removed the NEWS/What's New entries; I guess this can be considered an internal change.

serhiy-storchaka · 2023-08-18T07:52:23Z

Modules/_sqlite/connection.c

+#if SIZEOF_SIZE_T > 8
+        if (sz > 0xffffffffffffffff) {
            PyErr_SetString(PyExc_OverflowError,
-                            "string is longer than INT_MAX bytes");
+                            "Error setting result value: "
+                            "string is longer than 2**64-1 bytes");
            return -1;
        }
-        sqlite3_result_text(context, str, (int)sz, SQLITE_TRANSIENT);
+#endif


I wrote it. 😉

Anyway, it exclude from compilation simpler code.

What I propose here, to write either:

if ((size_t)sz > PYSQLITE_MAX_SIZE) {

where PYSQLITE_MAX_SIZE is defined as 0x7fffffffu (the current limit) or 0x7fffffffffffffffu (theoretical maximum), or

if ((sqlite3_uint64)sz != (size_t)sz) {

which just test the safety of integer conversion.

serhiy-storchaka · 2023-08-18T08:30:13Z

Lib/test/test_sqlite3/test_types.py

-    @unittest.skipUnless(sys.maxsize > 2**32, 'requires 64bit platform')
-    @support.bigmemtest(size=2**31, memuse=4, dry_run=False)
+    @unittest.skipUnless(sys.maxsize > 2**64, 'requires (s)size_t > 64bit')
+    @support.bigmemtest(size=2**64+1, memuse=4, dry_run=False)


What happens if left these tests as is?

…o sqlite/64-bit

erlend-aasland · 2023-09-04T13:38:25Z

I'm marking this as a draft; after all it's too experimental to consider for landing in its current form.

erlend-aasland · 2023-10-11T21:00:05Z

I don't have the time nor interest to follow this through. Feel free to pick it up.

erlend-aasland added 4 commits August 17, 2023 10:11

Use sqlite3_bind_blob64() iso. sqlite3_bind_blob()

83f2326

Use sqlite3_result_blob64() iso. sqlite3_result_blob()

91b09e3

Use sqlite3_bind_text64() iso. sqlite3_bind_text()

a94b8c7

Use sqlite3_result_text64() iso. sqlite3_result_text()

5d4312c

erlend-aasland requested a review from berkerpeksag as a code owner August 17, 2023 08:18

bedevere-bot mentioned this pull request Aug 17, 2023

sqlite3: use 64-bit APIs where possible #108021

Closed

bedevere-bot added the awaiting core review label Aug 17, 2023

erlend-aasland requested a review from serhiy-storchaka August 17, 2023 08:18

erlend-aasland added 2 commits August 17, 2023 10:23

Add NEWS and What's New items

24528d8

Adjust tests

d669736

serhiy-storchaka reviewed Aug 17, 2023

View reviewed changes

erlend-aasland and others added 3 commits August 17, 2023 11:09

Update Doc/whatsnew/3.13.rst

d25d920

Pull in main

13a709c

WIP

d76f7e3

serhiy-storchaka reviewed Aug 18, 2023

View reviewed changes

erlend-aasland added 4 commits September 4, 2023 15:04

Pull in main

a9ecce3

Simplify error messages

ce59534

Merge branch 'sqlite/64-bit' of github.com:erlend-aasland/cpython int…

6889b2e

…o sqlite/64-bit

Remove NEWS entries; considering this an internal change

32d12ef

erlend-aasland added the skip news label Sep 4, 2023

erlend-aasland marked this pull request as draft September 4, 2023 13:38

bedevere-bot removed the awaiting core review label Sep 4, 2023

erlend-aasland closed this Oct 11, 2023

erlend-aasland deleted the sqlite/64-bit branch October 11, 2023 21:00

Uh oh!

gh-108021: In sqlite3, use 64-bit APIs for binding and returning text and blobs #108075

gh-108021: In sqlite3, use 64-bit APIs for binding and returning text and blobs #108075

Uh oh!

Conversation

erlend-aasland commented Aug 17, 2023 • edited by bedevere-bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

erlend-aasland commented Aug 17, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

erlend-aasland commented Aug 17, 2023

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

erlend-aasland commented Sep 4, 2023

Uh oh!

erlend-aasland commented Oct 11, 2023

Uh oh!

Uh oh!

erlend-aasland commented Aug 17, 2023 •

edited by bedevere-bot

Loading

erlend-aasland commented Aug 17, 2023 •

edited

Loading