GeezerVison

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

GeezerVison is showing github.com — gh-99612: Fix PyUnicode_DecodeUTF8Stateful() for ASCII-only data by serhiy-storchaka · Pull Request #99613 · python/cpython

Conversation

serhiy-storchaka commented Nov 20, 2022 •

edited by bedevere-bot

Loading

Uh oh!

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

Copy link

Copy Markdown

Member

Previously *consumed was not set in this case.

This is an extraction from #99594.

Issue: PyUnicode_DecodeUTF8Stateful() does not set *consumed for ASCII-only string #99612

Sorry, something went wrong.

Uh oh!

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

All reactions


          pythongh-99612: Fix PyUnicode_DecodeUTF8Stateful() for ASCII-only data

8ff13de

Previously *consumed was not set in this case.

serhiy-storchaka added needs backport to 3.10 needs backport to 3.11 only security fixes labels

Nov 20, 2022

serhiy-storchaka requested a review from vstinner

November 20, 2022 07:56

bedevere-bot mentioned this pull request

Nov 20, 2022

PyUnicode_DecodeUTF8Stateful() does not set *consumed for ASCII-only string #99612

Closed

bedevere-bot added the awaiting core review label

Nov 20, 2022

vstinner reviewed

Nov 21, 2022

View reviewed changes

Comment thread

Lib/test/test_capi/test_unicode.py Outdated


		self.assertRaises(LookupError, decodeutf8, b'a\x80', 'foo')
		# TODO: Test PyUnicode_DecodeUTF8() with NULL as data and
		# negative size.

vstinner Nov 21, 2022

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.

I dislike TODO which stays forever. Please either address it or remove it.

Sorry, something went wrong.

Uh oh!

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

All reactions

serhiy-storchaka Nov 21, 2022

Copy link

Copy Markdown

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

In #99594 you requested to split that PR on smaller parts. If address these TODOes, it can add more complexity to this PR, which is not directly related to the bug.

Sorry, something went wrong.

Uh oh!

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

All reactions

serhiy-storchaka Nov 21, 2022

Copy link

Copy Markdown

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

If remove them, some cases will never be covered by tests.

Sorry, something went wrong.

Uh oh!

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

All reactions

Comment thread

Lib/test/test_capi/test_unicode.py


		class CAPITest(unittest.TestCase):

		@support.cpython_only

vstinner Nov 21, 2022

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.

What do you think of moving these tests to test_codecs? For me, these APIs are more related to codecs than "Unicode". test_codecs already has multiple tests about the UTF-8 encoding.

Sorry, something went wrong.

Uh oh!

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

All reactions

serhiy-storchaka Nov 21, 2022

Copy link

Copy Markdown

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

Done. But would not be more convenient to have tests for all PyUnicode_* functions in a single file?

Sorry, something went wrong.

Uh oh!

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

All reactions


          Move tests to test_capi.test_codecs.

b99be83

vstinner commented Nov 21, 2022

Copy link

Copy Markdown

Member

But would not be more convenient to have tests for all PyUnicode_* functions in a single file?

It would be nice to move the C code from the giant unicodeobject.c into a separed file, since codecs are not directly related to Unicode. They only "use" the Unicode API. I dislike the fact these functions share the PyUnicode_ prefix, but that cannot be changed now.

All reactions

Sorry, something went wrong.

Uh oh!

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

vstinner reviewed

Nov 21, 2022

View reviewed changes

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

Lib/test/test_capi/test_codecs.py

Oh. I was thinking at moving the tests in the existing Lib/test/test_codecs.py file.

See #93649 (comment) discussion in issue #93649.

Sorry, something went wrong.

Uh oh!

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

All reactions

serhiy-storchaka commented Nov 21, 2022

Copy link

Copy Markdown

Member Author

Lib/test/test_codecs.py is already too large, even without C API tests. It is better to split C API tests in a separate file.

Also, it is convenient to run all C API test if they are grouped together in a package.

All reactions

Sorry, something went wrong.

Uh oh!

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

vstinner mentioned this pull request

Nov 21, 2022

Split up _testcapimodule.c #93649

Open

vstinner commented Nov 21, 2022

Copy link

Copy Markdown

Member

For the organization of C API tests, I commented there: #93649 (comment)

All reactions

Sorry, something went wrong.

Uh oh!

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

serhiy-storchaka mentioned this pull request

Nov 29, 2022

gh-93649: Split unicode tests from _testcapimodule.c & add some more #95819

Merged


          Merge branch 'main' into capi-PyUnicode_DecodeUTF8Stateful

3d23080

vstinner reviewed

Nov 29, 2022

View reviewed changes

Comment thread

Lib/test/test_capi/test_codecs.py Outdated


		class CAPITest(unittest.TestCase):

		@support.cpython_only

vstinner Nov 29, 2022

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.

For me it's redundant with testing _testcapi. For example, PyPy doesn't have _testcapi. But you can keep it if you prefer to be explicit.

Sorry, something went wrong.

Uh oh!

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

All reactions

Comment thread

Lib/test/test_capi/test_codecs.py Outdated

		class CAPITest(unittest.TestCase):

		@support.cpython_only
		@unittest.skipIf(_testcapi is None, 'need _testcapi module')

vstinner Nov 29, 2022

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.

IMO you can move it on the whole class.

Sorry, something went wrong.

Uh oh!

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

All reactions

Comment thread

Lib/test/test_capi/test_codecs.py Outdated

		try:
		import _testcapi
		except ImportError:
		_testcapi = None

vstinner Nov 29, 2022

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.

Why not skipping this test if the module is missing?

Sorry, something went wrong.

Uh oh!

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

All reactions

Comment thread

Lib/test/test_capi/test_codecs.py Outdated

		@unittest.skipIf(_testcapi is None, 'need _testcapi module')
		def test_decodeutf8(self):
		"""Test PyUnicode_DecodeUTF8()"""
		from _testcapi import unicode_decodeutf8 as decodeutf8

vstinner Nov 29, 2022

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.

You may use decodeutf8 = _testcapi.unicode_decodeutf8.

Sorry, something went wrong.

Uh oh!

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

All reactions

Comment thread

Modules/_testcapi/unicode.c Outdated

		const char *data;
		Py_ssize_t size;
		const char *errors = NULL;
		Py_ssize_t consumed;

vstinner Nov 29, 2022

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.

I would feel safer if you initialize the value. Maybe to a marker value like 42?

Otherwise, the test may miss the bug by luck, if local variables allocated on the stack are initialize to 0.

Sorry, something went wrong.

Uh oh!

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

All reactions

serhiy-storchaka added 2 commits

November 30, 2022 18:51


          Merge branch 'main' into capi-PyUnicode_DecodeUTF8Stateful

1c07d9f


          Address review comments.

fd8c21d

vstinner approved these changes

Dec 1, 2022

View reviewed changes

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

LGTM, thanks for updating the PR. I'm more confident with the consumed variable initialized to a value in the test ;-)

Sorry, something went wrong.

Uh oh!

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

All reactions

bedevere-bot added awaiting merge and removed awaiting core review labels

Dec 1, 2022

serhiy-storchaka commented Dec 1, 2022 •

edited

Loading

Uh oh!

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

Copy link

Copy Markdown

Member Author

I completely agree with you.

All reactions

Sorry, something went wrong.

Uh oh!

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

serhiy-storchaka merged commit f08e52c into python:main

Dec 1, 2022

miss-islington commented Dec 1, 2022

Copy link

Copy Markdown

Contributor

Thanks @serhiy-storchaka for the PR 🌮🎉.. I'm working now to backport this PR to: 3.10, 3.11.
🐍🍒⛏🤖

All reactions

Sorry, something went wrong.

Uh oh!

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

bedevere-bot removed the awaiting merge label

Dec 1, 2022

miss-islington commented Dec 1, 2022

Copy link

Copy Markdown

Contributor

Sorry, @serhiy-storchaka, I could not cleanly backport this to 3.11 due to a conflict.
Please backport using cherry_picker on command line.
cherry_picker f08e52ccb027f6f703302b8c1a82db9fd3934270 3.11

All reactions

Sorry, something went wrong.

Uh oh!

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

miss-islington assigned serhiy-storchaka

Dec 1, 2022

miss-islington commented Dec 1, 2022

Copy link

Copy Markdown

Contributor

Sorry @serhiy-storchaka, I had trouble checking out the 3.10 backport branch.
Please retry by removing and re-adding the "needs backport to 3.10" label.
Alternatively, you can backport using cherry_picker on the command line.
cherry_picker f08e52ccb027f6f703302b8c1a82db9fd3934270 3.10

All reactions

Sorry, something went wrong.

Uh oh!

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

carljm added a commit to carljm/cpython that referenced this pull request

Dec 1, 2022


          Merge branch 'main' into fixinspect

12f7659

* main: (112 commits)
  pythongh-99894: Ensure the local names don't collide with the test file in traceback suggestion error checking (python#99895)
  pythongh-99612: Fix PyUnicode_DecodeUTF8Stateful() for ASCII-only data (pythonGH-99613)
  Doc: Add summary line to isolation_level & autocommit sqlite3.connect params (python#99917)
  pythonGH-98906 ```re``` module: ```search() vs. match()``` section should mention ```fullmatch()``` (pythonGH-98916)
  pythongh-89189: More compact range iterator (pythonGH-27986)
  bpo-47220: Document the optional callback parameter of weakref.WeakMethod (pythonGH-25491)
  pythonGH-99905: Fix output of misses in summarize_stats.py execution counts (pythonGH-99906)
  pythongh-99845: PEP 670: Convert PyObject macros to functions (python#99850)
  pythongh-99845: Use size_t type in __sizeof__() methods (python#99846)
  pythonGH-99877)
  Fix typo in exception message in `multiprocessing.pool` (python#99900)
  pythongh-87092: move all localsplus preparation into separate function called from assembler stage (pythonGH-99869)
  pythongh-99891: Fix infinite recursion in the tokenizer when showing warnings (pythonGH-99893)
  pythongh-99824: Document that sqlite3.connect implicitly open a transaction if autocommit=False (python#99825)
  pythonGH-81057: remove static state from suggestions.c (python#99411)
  Improve zip64 limit error message (python#95892)
  pythongh-98253: Break potential reference cycles in external code worsened by typing.py lru_cache (python#98591)
  pythongh-99127: Allow some features of syslog to the main interpreter only (pythongh-99128)
  pythongh-82836: fix private network check (python#97733)
  Docs: improve accuracy of socketserver reference (python#24767)
  ...

serhiy-storchaka deleted the capi-PyUnicode_DecodeUTF8Stateful branch

July 25, 2023 09:50

serhiy-storchaka added a commit to serhiy-storchaka/cpython that referenced this pull request

Jul 25, 2023


          [3.11] pythongh-99612: Fix PyUnicode_DecodeUTF8Stateful() for ASCII-o…

c1d4f11

…nly data (pythonGH-99613)

Previously *consumed was not set in this case..
(cherry picked from commit f08e52c)

Co-authored-by: Serhiy Storchaka <[email protected]>

bedevere-bot commented Jul 25, 2023

Copy link

Copy Markdown

GH-107224 is a backport of this pull request to the 3.11 branch.

All reactions

Sorry, something went wrong.

Uh oh!

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

bedevere-bot removed the needs backport to 3.11 only security fixes label

Jul 25, 2023

serhiy-storchaka removed their assignment

Jul 25, 2023

serhiy-storchaka removed the needs backport to 3.10 label

Jul 25, 2023

serhiy-storchaka added a commit that referenced this pull request

Jul 25, 2023


          [3.11] gh-99612: Fix PyUnicode_DecodeUTF8Stateful() for ASCII-only da…

b8b3e6a

…ta (GH-99613) (GH-107224)

Previously *consumed was not set in this case.
(cherry picked from commit f08e52c)

miss-islington pushed a commit to miss-islington/cpython that referenced this pull request

Jul 25, 2023


          [3.11] pythongh-99612: Fix PyUnicode_DecodeUTF8Stateful() for ASCII-o…

5b13e86

…nly data (pythonGH-99613) (pythonGH-107224)

(cherry picked from commit b8b3e6a)

Co-authored-by: Serhiy Storchaka <[email protected]>
Previously *consumed was not set in this case.
(cherry picked from commit f08e52c)

serhiy-storchaka added a commit to serhiy-storchaka/cpython that referenced this pull request

Jul 25, 2023


          [3.9] [3.11] pythongh-99612: Fix PyUnicode_DecodeUTF8Stateful() for A…

f72f15d

…SCII-only data (pythonGH-99613) (pythonGH-107224)

Previously *consumed was not set in this case.
(cherry picked from commit f08e52c).
(cherry picked from commit b8b3e6a)

Co-authored-by: Serhiy Storchaka <[email protected]>

ambv pushed a commit that referenced this pull request

Aug 22, 2023


          [3.10] gh-99612: Fix PyUnicode_DecodeUTF8Stateful() for ASCII-only da…

1c937e5

…ta (GH-99613) (GH-107224) (#107230)

Previously *consumed was not set in this case.

(cherry picked from commit b8b3e6a)
(cherry picked from commit f08e52c)

Co-authored-by: Serhiy Storchaka <[email protected]>

ambv pushed a commit that referenced this pull request

Aug 22, 2023


          [3.9] gh-99612: Fix PyUnicode_DecodeUTF8Stateful() for ASCII-only data (

4a79328

GH-99613) (GH-107224) (#107231)

Previously *consumed was not set in this case.
(cherry picked from commit f08e52c).
(cherry picked from commit b8b3e6a)

carlosroman pushed a commit to DataDog/cpython that referenced this pull request

Oct 11, 2023


          [3.9] pythongh-99612: Fix PyUnicode_DecodeUTF8Stateful() for ASCII-on…

0bd646a

…ly data (pythonGH-99613) (pythonGH-107224) (python#107231)

Previously *consumed was not set in this case.
(cherry picked from commit f08e52c).
(cherry picked from commit b8b3e6a)

Reviewers

vstinner

vstinner approved these changes

Uh oh!

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

Assignees

No one assigned

Labels

None yet

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.

4 participants

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.

Conversation

serhiy-storchaka commented Nov 20, 2022 • edited by bedevere-bot Loading Uh oh! There was an error while loading. Please reload this page.

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!

vstinner commented Nov 21, 2022

Uh oh!

vstinner left a comment

Choose a reason for hiding this comment

Uh oh!

serhiy-storchaka commented Nov 21, 2022

Uh oh!

vstinner commented Nov 21, 2022

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!

vstinner left a comment

Choose a reason for hiding this comment

Uh oh!

serhiy-storchaka commented Dec 1, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

miss-islington commented Dec 1, 2022

Uh oh!

miss-islington commented Dec 1, 2022

Uh oh!

miss-islington commented Dec 1, 2022

Uh oh!

bedevere-bot commented Jul 25, 2023

Uh oh!

Reviewers

Uh oh!

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

serhiy-storchaka commented Nov 20, 2022 •

edited by bedevere-bot

Loading

Uh oh!

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

serhiy-storchaka commented Dec 1, 2022 •

edited

Loading

Uh oh!

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