bpo-44822: Don't truncate `str`s with embedded NULL chars returned by `sqlite3` UDF callbacks#27588

erlend-aasland · 2021-08-04T08:33:54Z

https://bugs.python.org/issue44822

erlend-aasland · 2021-08-04T10:22:41Z

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.

Lib/sqlite3/test/userfunctions.py

erlend-aasland · 2021-08-04T14:29:56Z

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 · 2021-08-04T15:24:41Z

Misc/NEWS.d/next/Library/2021-08-04-12-29-00.bpo-44822.zePNXA.rst

@@ -0,0 +1,3 @@
+:mod:`sqlite3` now raises :exc:`OverflowError` if an user-defined function


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`.

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

bedevere-bot · 2021-08-04T15:25:07Z

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 · 2021-08-04T22:19:29Z

I have made the requested changes; please review again.

Misc/NEWS.d/next/Library/2021-08-04-12-29-00.bpo-44822.zePNXA.rst

miss-islington · 2021-08-05T07:22:11Z

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 · 2021-08-05T07:22:19Z

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 · 2021-08-05T07:22:21Z

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>

erlend-aasland · 2021-08-05T07:25:07Z

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

erlend-aasland · 2021-08-05T08:03:28Z

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 · 2021-08-06T11:49:21Z

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

👍

… `sqlite3` UDF callbacks (GH-27588) (cherry picked from commit 8f010dc) Co-authored-by: Erlend Egeberg Aasland <erlend.aasland@innova.no>

miss-islington · 2021-08-06T18:02:00Z

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 · 2021-08-06T18:02:06Z

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 · 2021-08-06T18:53:00Z

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>

…ned by `sqlite3` UDF callbacks (GH-27588). (GH-27639) (cherry picked from commit 8f010dc)

bpo-44822: Pass string size to sqlite3_result_text()
2241795

erlend-aasland requested a review from berkerpeksag as a code owner August 4, 2021 08:33

bedevere-bot added the awaiting review label Aug 4, 2021

the-knights-who-say-ni added the CLA signed label Aug 4, 2021

erlend-aasland added the skip news label Aug 4, 2021

Erlend E. Aasland added 2 commits August 4, 2021 11:07

Raise OverflowError if size > INT_MAX
58d3244

Add test
73639f1

erlend-aasland force-pushed the sqlite-result-text-size branch from 45cf442 to 73639f1Compare August 4, 2021 10:01

erlend-aasland requested a review from serhiy-storchaka August 4, 2021 10:02

serhiy-storchaka added needs backport to 3.9 needs backport to 3.10 only security fixes type-bug An unexpected behavior, bug, or error and removed skip news labels Aug 4, 2021

Erlend E. Aasland added 2 commits August 4, 2021 12:29

Add NEWS entry
60d94f9

Don't capitalize error string
10fbc4b

serhiy-storchaka reviewed Aug 4, 2021
View reviewed changes

Lib/sqlite3/test/userfunctions.pyShow resolvedHide resolved

Add aggregate test
a483052

serhiy-storchaka approved these changes Aug 4, 2021
View reviewed changes

bedevere-bot added awaiting merge and removed awaiting review labels Aug 4, 2021

serhiy-storchaka requested changes Aug 4, 2021
View reviewed changes

bedevere-bot removed the awaiting merge label Aug 4, 2021

bedevere-bot added the awaiting changes label Aug 4, 2021

Address review: reword NEWS entry
55963ca

erlend-aasland force-pushed the sqlite-result-text-size branch from 9a54953 to 55963caCompare August 4, 2021 22:17

erlend-aasland changed the title ~~bpo-44822: Pass string size to sqlite3_result_text()~~bpo-44822: Don't truncate strs with embedded NULL chars returned by sqlite3 UDF callbacksAug 4, 2021

serhiy-storchaka reviewed Aug 5, 2021
View reviewed changes

Misc/NEWS.d/next/Library/2021-08-04-12-29-00.bpo-44822.zePNXA.rst Outdated Show resolvedHide resolved

Apply suggestions from code review
7a2c279

serhiy-storchaka approved these changes Aug 5, 2021
View reviewed changes

bedevere-bot added awaiting merge and removed awaiting change review labels Aug 5, 2021

serhiy-storchaka merged commit 8f010dc into python:mainAug 5, 2021

bedevere-bot removed the awaiting merge label Aug 5, 2021

miss-islington assigned serhiy-storchaka Aug 5, 2021

bedevere-bot removed the needs backport to 3.10 only security fixes label Aug 5, 2021

erlend-aasland deleted the sqlite-result-text-size branch August 5, 2021 07:24

miss-islington added a commit that referenced this pull request Aug 6, 2021
bpo-44822: Don't truncatestrs with embedded NULL chars returned by…
2b1e713
… `sqlite3` UDF callbacks (GH-27588) (cherry picked from commit 8f010dc) Co-authored-by: Erlend Egeberg Aasland <erlend.aasland@innova.no>

serhiy-storchaka added needs backport to 3.9 and removed needs backport to 3.9 labels Aug 6, 2021

bedevere-bot removed the needs backport to 3.9 label Aug 6, 2021

erlend-aasland mentioned this pull request Aug 6, 2021
bpo-43853: Expand test suite for SQLite UDF's #27642
Merged

serhiy-storchaka pushed a commit that referenced this pull request Aug 6, 2021
[3.9]bpo-44822: Don't truncatestrs with embedded NULL chars retur…
c352412
…ned by `sqlite3` UDF callbacks (GH-27588). (GH-27639) (cherry picked from commit 8f010dc)

erlend-aasland mentioned this pull request May 16, 2022
[sqlite3] Do not truncate results of user functions and aggregates on the first NUL #88985
Closed

serhiy-storchaka removed their assignment Jun 21, 2022

		@@ -0,0 +1,3 @@
		:mod:`sqlite3` now raises :exc:`OverflowError` if an user-defined function

	: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`.

Uh oh!

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

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

Uh oh!

Conversation

erlend-aasland commented Aug 4, 2021• 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 4, 2021• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

serhiy-storchaka left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

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

Uh oh!

Uh oh!

serhiy-storchaka left a comment

Choose a reason for hiding this comment

Uh oh!

serhiy-storchakaAug 4, 2021

Choose a reason for hiding this comment

Uh oh!

erlend-aaslandAug 4, 2021

Choose a reason for hiding this comment

Uh oh!

Uh oh!

bedevere-bot commented Aug 4, 2021

Uh oh!

erlend-aasland commented Aug 4, 2021

Uh oh!

Uh oh!

miss-islington commented Aug 5, 2021

Uh oh!

miss-islington commented Aug 5, 2021

Uh oh!

bedevere-bot commented Aug 5, 2021

Uh oh!

erlend-aasland commented Aug 5, 2021

Uh oh!

erlend-aasland commented Aug 5, 2021

Uh oh!

serhiy-storchaka commented Aug 6, 2021

Uh oh!

miss-islington commented Aug 6, 2021

Uh oh!

miss-islington commented Aug 6, 2021

Uh oh!

bedevere-bot commented Aug 6, 2021

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

bpo-44822: Don't truncate `str`s with embedded NULL chars returned by `sqlite3` UDF callbacks#27588

bpo-44822: Don't truncate `str`s with embedded NULL chars returned by `sqlite3` UDF callbacks #27588

erlend-aasland commented Aug 4, 2021•
edited by bedevere-bot
Loading

erlend-aasland commented Aug 4, 2021•
edited
Loading

serhiy-storchaka left a comment •
edited
Loading

erlend-aasland commented Aug 4, 2021•
edited
Loading