GV

GeezerVison

The World Wide Web, restored to 800 pixels of glory.

Home|About|Gaming cabinet|Archives|How it works|Starter sites

On this site

  • Sign in
  • ActionsCI/CD and automation
  • CodespacesCloud development environments
  • All Features
  • Documentation
  • Training
  • Certifications
  • Build with Copilot
  • Customize Copilot
  • Integrations
  • Trending repos
  • GitHub Sponsors
  • Developer articles
  • Enterprises

This view

  • Original page
  • New address

GeezerVison is showing github.com — bpo-44822: Don't truncate `str`s with embedded NULL chars returned by `sqlite3` UDF callbacks by erlend-aasland · Pull Request #27588 · python/cpython

Conversation

@erlend-aasland

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

Uh oh!

There was an error while loading. Please reload this page.

Copy link
Copy Markdown
Contributor

https://bugs.python.org/issue44822

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions
Erlend E. Aasland
bpo-44822: Pass string size to sqlite3_result_text()
2241795
@erlend-aasland
erlend-aasland requested a review from berkerpeksag as a code owner August 4, 2021 08:33
@bedevere-bot bedevere-bot added the awaiting review label Aug 4, 2021
@the-knights-who-say-ni the-knights-who-say-ni added the CLA signed label Aug 4, 2021
@erlend-aasland erlend-aasland added the skip news label Aug 4, 2021
Erlend E. Aasland added 2 commits August 4, 2021 11:07
Erlend E. Aasland
Raise OverflowError if size > INT_MAX
58d3244
Erlend E. Aasland
Add test
73639f1
@erlend-aasland
erlend-aasland force-pushed the sqlite-result-text-size branch from 45cf442 to 73639f1 Compare August 4, 2021 10:01
@erlend-aasland
erlend-aasland requested a review from serhiy-storchaka August 4, 2021 10:02
@serhiy-storchaka serhiy-storchaka added needs backport to 3.9 type-bug An unexpected behavior, bug, or error and removed skip news labels Aug 4, 2021
@erlend-aasland

erlend-aasland commented Aug 4, 2021 •
edited
Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link
Copy Markdown
Contributor Author

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

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

Erlend E. Aasland added 2 commits August 4, 2021 12:29
Erlend E. Aasland
Add NEWS entry
60d94f9
Erlend E. Aasland
Don't capitalize error string
10fbc4b
serhiy-storchaka
serhiy-storchaka reviewed Aug 4, 2021
View reviewed changes

@serhiy-storchaka serhiy-storchaka left a comment •
edited
Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link
Copy Markdown
Member

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.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions
serhiy-storchaka
serhiy-storchaka reviewed Aug 4, 2021
View reviewed changes
Comment thread Lib/sqlite3/test/userfunctions.py Resolved

Uh oh!

There was an error while loading. Please reload this page.

Erlend E. Aasland
Add aggregate test
a483052
@erlend-aasland

erlend-aasland commented Aug 4, 2021 •
edited
Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link
Copy Markdown
Contributor Author

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.

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

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

@serhiy-storchaka serhiy-storchaka left a comment

Copy link
Copy Markdown
Member

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.

👍

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions
@bedevere-bot bedevere-bot added awaiting merge and removed awaiting review labels Aug 4, 2021
serhiy-storchaka
serhiy-storchaka requested changes Aug 4, 2021
View reviewed changes
Comment thread Misc/NEWS.d/next/Library/2021-08-04-12-29-00.bpo-44822.zePNXA.rst Outdated
@@ -0,0 +1,3 @@
:mod:`sqlite3` now raises :exc:`OverflowError` if an user-defined function

@serhiy-storchaka serhiy-storchaka Aug 4, 2021

Copy link
Copy Markdown
Member

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):

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

@erlend-aasland erlend-aasland Aug 4, 2021

Copy link
Copy Markdown
Contributor Author

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?

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions
Comment thread Misc/NEWS.d/next/Library/2021-08-04-12-29-00.bpo-44822.zePNXA.rst Resolved

Uh oh!

There was an error while loading. Please reload this page.

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

bedevere-bot commented Aug 4, 2021

Copy link
Copy Markdown

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.

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@bedevere-bot bedevere-bot added the awaiting changes label Aug 4, 2021
Erlend E. Aasland
Address review: reword NEWS entry
55963ca
@erlend-aasland
erlend-aasland force-pushed the sqlite-result-text-size branch from 9a54953 to 55963ca Compare August 4, 2021 22:17
@erlend-aasland

erlend-aasland commented Aug 4, 2021

Copy link
Copy Markdown
Contributor Author

I have made the requested changes; please review again.

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@erlend-aasland 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 callbacks Aug 4, 2021
serhiy-storchaka
serhiy-storchaka reviewed Aug 5, 2021
View reviewed changes
Comment thread Misc/NEWS.d/next/Library/2021-08-04-12-29-00.bpo-44822.zePNXA.rst Resolved

Uh oh!

There was an error while loading. Please reload this page.

@serhiy-storchaka
Apply suggestions from code review
7a2c279
serhiy-storchaka
serhiy-storchaka approved these changes Aug 5, 2021
View reviewed changes
@bedevere-bot bedevere-bot added awaiting merge and removed awaiting change review labels Aug 5, 2021
@serhiy-storchaka
serhiy-storchaka merged commit 8f010dc into python:main Aug 5, 2021
@miss-islington

miss-islington commented Aug 5, 2021

Copy link
Copy Markdown
Contributor

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.
🐍🍒⛏🤖

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

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

miss-islington commented Aug 5, 2021

Copy link
Copy Markdown
Contributor

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

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@miss-islington miss-islington assigned serhiy-storchaka Aug 5, 2021
@bedevere-bot bedevere-bot removed the needs backport to 3.10 label Aug 5, 2021
@bedevere-bot

bedevere-bot commented Aug 5, 2021

Copy link
Copy Markdown

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

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

miss-islington pushed a commit to miss-islington/cpython that referenced this pull request Aug 5, 2021
Erlend Egeberg Aasland @miss-islington
bpo-44822: Don't truncate strs with embedded NULL chars returned by…
c5aa1b8
… `sqlite3` UDF callbacks (pythonGH-27588)

(cherry picked from commit 8f010dc)

Co-authored-by: Erlend Egeberg Aasland <[email protected]>
@erlend-aasland
erlend-aasland deleted the sqlite-result-text-size branch August 5, 2021 07:24
@erlend-aasland

erlend-aasland commented Aug 5, 2021

Copy link
Copy Markdown
Contributor Author

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

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@erlend-aasland

erlend-aasland commented Aug 5, 2021

Copy link
Copy Markdown
Contributor Author

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.

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@serhiy-storchaka

serhiy-storchaka commented Aug 6, 2021

Copy link
Copy Markdown
Member

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

👍

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

miss-islington added a commit that referenced this pull request Aug 6, 2021
@miss-islington Erlend Egeberg Aasland
bpo-44822: Don't truncate strs with embedded NULL chars returned by…
2b1e713
… `sqlite3` UDF callbacks (GH-27588)

(cherry picked from commit 8f010dc)

Co-authored-by: Erlend Egeberg Aasland <[email protected]>
@serhiy-storchaka serhiy-storchaka added needs backport to 3.9 and removed needs backport to 3.9 labels Aug 6, 2021
@miss-islington

miss-islington commented Aug 6, 2021

Copy link
Copy Markdown
Contributor

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

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@miss-islington

miss-islington commented Aug 6, 2021

Copy link
Copy Markdown
Contributor

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

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@bedevere-bot

bedevere-bot commented Aug 6, 2021

Copy link
Copy Markdown

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

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@bedevere-bot bedevere-bot removed the needs backport to 3.9 label Aug 6, 2021
erlend-aasland pushed a commit to erlend-aasland/cpython that referenced this pull request Aug 6, 2021
Erlend Egeberg Aasland
[3.9] bpo-44822: Don't truncate strs with embedded NULL chars retur…
6cf0d64
…ned by `sqlite3` UDF callbacks (pythonGH-27588).

(cherry picked from commit 8f010dc)

Co-authored-by: Erlend Egeberg Aasland <[email protected]>
@erlend-aasland 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
Erlend Egeberg Aasland
[3.9] bpo-44822: Don't truncate strs with embedded NULL chars retur…
c352412
…ned by `sqlite3` UDF callbacks (GH-27588). (GH-27639)

(cherry picked from commit 8f010dc)
@erlend-aasland 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 serhiy-storchaka removed their assignment Jun 21, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@serhiy-storchaka serhiy-storchaka serhiy-storchaka approved these changes
@berkerpeksag berkerpeksag Awaiting requested review from berkerpeksag

Uh oh!

There was an error while loading. Please reload this page.

Assignees

No one assigned

Labels

type-bug An unexpected behavior, bug, or error

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Uh oh!

There was an error while loading. Please reload this page.

5 participants

@erlend-aasland @bedevere-bot @miss-islington @serhiy-storchaka @the-knights-who-say-ni
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
You are visitor 01402503 — GeezerVison does not store the pages you look up.
Best viewed with Netscape Navigator 4.7 or Internet Explorer 6 at 800×600. No cookies required.