bpo-44822: Don't truncate strs with embedded NULL chars returned by sqlite3 UDF callbacks

[erlend-aasland]

https://bugs.python.org/issue44822

[erlend-aasland]

Ah, yes, we need a news entry now that we raise OverflowError.

[serhiy-storchaka]

[x] Please add a NEWS entry. It is now a bug fix, not a merely micro-optimization.

Would be nice to add also tests for strings with lengths INT_MAX and INT_MAX+1. I am not sure SQLite works with INT_MAX, it will likely raise an error. Use the bigmemtest decorator from test.support, because tests will consume at least 2GiB of memory (maybe more if the case with INT_MAX passes).

If you have problems with writing such tests (if they require much more than 4GiB of memory), no problem, I'll write them.

[erlend-aasland]

Would be nice to add also tests for strings with lengths INT_MAX and INT_MAX+1. I am not sure SQLite works with INT_MAX, it will likely raise an error. Use the bigmemtest decorator from test.support, because tests will consume at least 2GiB of memory (maybe more if the case with INT_MAX passes).

If you have problems with writing such tests (if they require much more than 4GiB of memory), no problem, I'll write them.

Nice, I was unaware of that support function.

I've added tests with _2G and _4G strings, but that will only work for 32-bit ints. Here's my first try (it won't run on my computer, so I haven't tested it yet):

diff --git a/Lib/sqlite3/test/userfunctions.py b/Lib/sqlite3/test/userfunctions.py
index b3da3c425b..6d72295491 100644
--- a/Lib/sqlite3/test/userfunctions.py
+++ b/Lib/sqlite3/test/userfunctions.py
@@ -31,0 +32,2 @@
+from test.support import bigmemtest, _2G, _4G
+
@@ -231,0 +234,9 @@ def test_func_return_text_with_null_char(self):
+    @bigmemtest(size=_4G, memuse=1, dry_run=False)
+    def test_func_bigmem_text(self):
+        cur = self.con.cursor()
+        self.con.create_function("bigmem2g", 0, lambda: "b" * _2G)
+        self.con.create_function("bigmem4g", 0, lambda: "b" * _4G)
+        res = cur.execute("select bigmem2g()").fetchone()[0]
+        self.assertEqual(len(res), _2G)
+        self.assertRaises(OverflowError, cur.execute, "select bigmem4g()")
+

Feel free to add the tests if you are able to run bigmem tests locally.

[serhiy-storchaka]

👍

[serhiy-storchaka]

Actually sqlite3 overwrites OverflowError with other exception. But it does not matter, the most important part of this PR is that result strings with NUL are now correctly supported.

Something like (please correct my English):

Suggested change

	:mod:`sqlite3` now raises :exc:`OverflowError` if an user-defined function
	Fix support of user functions and aggregates returning string containing NUL in :mod:`sqlite3`.

[erlend-aasland]

Actually sqlite3 overwrites OverflowError with other exception.

Ah, yes, there should be some exception chaining in the UDF callbacks. I'll open an issue for it.

Anyway, how does the reworded NEWS entry look?

[bedevere-bot]

A Python core developer has requested some changes be made to your pull request before we can consider merging it. If you could please address their requests along with any other requests in other reviews from core developers that would be appreciated.

Once you have made the requested changes, please leave a comment on this pull request containing the phrase I have made the requested changes; please review again. I will then notify any core developers who have left a review that you're ready for them to take another look at this pull request.

[erlend-aasland]

I have made the requested changes; please review again.

[miss-islington]

Thanks @erlend-aasland for the PR, and @serhiy-storchaka for merging it 🌮🎉.. I'm working now to backport this PR to: 3.10, 3.9.
🐍🍒⛏🤖

[miss-islington]

Sorry, @erlend-aasland and @serhiy-storchaka, I could not cleanly backport this to 3.9 due to a conflict.
Please backport using cherry_picker on command line.
cherry_picker 8f010dc920e1f6dc6a357e7cc1460a7a567c05c6 3.9

[bedevere-bot]

GH-27611 is a backport of this pull request to the 3.10 branch.

[erlend-aasland]

Thank you for your review and suggestions, Serhiy. Much appreciated!

[erlend-aasland]

In order to backport this to 3.9, we also need to backport #25422. Either we first backport #25422 to 3.9 and then backport this to 3.9, or we don't backport this PR to 3.9.

Let me know what you think, @serhiy-storchaka.

[serhiy-storchaka]

In order to backport this to 3.9, we also need to backport #25422.

👍

[miss-islington]

Thanks @erlend-aasland for the PR, and @serhiy-storchaka for merging it 🌮🎉.. I'm working now to backport this PR to: 3.9.
🐍🍒⛏🤖

[miss-islington]

Sorry, @erlend-aasland and @serhiy-storchaka, I could not cleanly backport this to 3.9 due to a conflict.
Please backport using cherry_picker on command line.
cherry_picker 8f010dc920e1f6dc6a357e7cc1460a7a567c05c6 3.9

[bedevere-bot]

GH-27639 is a backport of this pull request to the 3.9 branch.