-
-
Notifications
You must be signed in to change notification settings - Fork 31.8k
bpo-44822: Don't truncate str
s with embedded NULL chars returned by sqlite3
UDF callbacks
#27588
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
bpo-44822: Don't truncate str
s with embedded NULL chars returned by sqlite3
UDF callbacks
#27588
Conversation
45cf442
to
73639f1
Compare
Ah, yes, we need a news entry now that we raise |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
[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.
Nice, I was unaware of that support function. I've added tests with 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. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍
@@ -0,0 +1,3 @@ | |||
:mod:`sqlite3` now raises :exc:`OverflowError` if an user-defined function |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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):
: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`. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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?
Misc/NEWS.d/next/Library/2021-08-04-12-29-00.bpo-44822.zePNXA.rst
Outdated
Show resolved
Hide resolved
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 |
9a54953
to
55963ca
Compare
I have made the requested changes; please review again. |
str
s with embedded NULL chars returned by sqlite3
UDF callbacks
Misc/NEWS.d/next/Library/2021-08-04-12-29-00.bpo-44822.zePNXA.rst
Outdated
Show resolved
Hide resolved
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. |
Sorry, @erlend-aasland and @serhiy-storchaka, I could not cleanly backport this to |
GH-27611 is a backport of this pull request to the 3.10 branch. |
… `sqlite3` UDF callbacks (pythonGH-27588) (cherry picked from commit 8f010dc) Co-authored-by: Erlend Egeberg Aasland <erlend.aasland@innova.no>
Thank you for your review and suggestions, Serhiy. Much appreciated! |
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. |
👍 |
Thanks @erlend-aasland for the PR, and @serhiy-storchaka for merging it 🌮🎉.. I'm working now to backport this PR to: 3.9. |
Sorry, @erlend-aasland and @serhiy-storchaka, I could not cleanly backport this to |
GH-27639 is a backport of this pull request to the 3.9 branch. |
…ned by `sqlite3` UDF callbacks (pythonGH-27588). (cherry picked from commit 8f010dc) Co-authored-by: Erlend Egeberg Aasland <erlend.aasland@innova.no>
https://bugs.python.org/issue44822