gh-99612: Fix PyUnicode_DecodeUTF8Stateful() for ASCII-only data by serhiy-storchaka · Pull Request #99613 · python/cpython · GitHub

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

Previously *consumed was not set in this case.

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

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.

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

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.

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

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.

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.

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.

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

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.

IMO you can move it on the whole class.

Why not skipping this test if the module is missing?

You may use decodeutf8 = _testcapi.unicode_decodeutf8.

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.

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

I completely agree with you.

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

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

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

* 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) ...

…nly data (pythonGH-99613) Previously *consumed was not set in this case.. (cherry picked from commit f08e52c) Co-authored-by: Serhiy Storchaka <storchaka@gmail.com>

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

…ta (GH-99613) (GH-107224) Previously *consumed was not set in this case. (cherry picked from commit f08e52c)

…nly data (pythonGH-99613) (pythonGH-107224) (cherry picked from commit b8b3e6a) Co-authored-by: Serhiy Storchaka <storchaka@gmail.com> Previously *consumed was not set in this case. (cherry picked from commit f08e52c)

…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 <storchaka@gmail.com>

…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 <storchaka@gmail.com>

GH-99613) (GH-107224) (#107231) Previously *consumed was not set in this case. (cherry picked from commit f08e52c). (cherry picked from commit b8b3e6a)

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

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 only security fixes 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

Move tests to test_capi.test_codecs.

b99be83

vstinner reviewed Nov 21, 2022

View reviewed changes

vstinner mentioned this pull request Nov 21, 2022

Split up _testcapimodule.c #93649

Open

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

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

bedevere-bot added awaiting merge and removed awaiting core review labels Dec 1, 2022

serhiy-storchaka merged commit f08e52c into python:main Dec 1, 2022

bedevere-bot removed the awaiting merge label Dec 1, 2022

miss-islington assigned serhiy-storchaka Dec 1, 2022

serhiy-storchaka deleted the capi-PyUnicode_DecodeUTF8Stateful branch July 25, 2023 09:50

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 only security fixes 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)

+              try:
+                  import _testcapi
+              except ImportError:
+                  _testcapi = None

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

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

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

+              class CAPITest(unittest.TestCase):
+                  @support.cpython_only
+                  @unittest.skipIf(_testcapi is None, 'need _testcapi module')

gh-99612: Fix PyUnicode_DecodeUTF8Stateful() for ASCII-only data by serhiy-storchaka · Pull Request #99613 · python/cpython · GitHub

Uh oh!

Uh oh!

gh-99612: Fix PyUnicode_DecodeUTF8Stateful() for ASCII-only data - #99613

gh-99612: Fix PyUnicode_DecodeUTF8Stateful() for ASCII-only data#99613serhiy-storchaka merged 5 commits intopython:mainpython/cpython:mainfrom serhiy-storchaka:capi-PyUnicode_DecodeUTF8Statefulserhiy-storchaka/cpython:capi-PyUnicode_DecodeUTF8StatefulCopy head branch name to clipboard

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

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

gh-99612: Fix PyUnicode_DecodeUTF8Stateful() for ASCII-only data#99613
serhiy-storchaka merged 5 commits into
python:mainfrom
serhiy-storchaka:capi-PyUnicode_DecodeUTF8Stateful

serhiy-storchaka commented Nov 20, 2022 •
edited by bedevere-bot
Loading

serhiy-storchaka commented Dec 1, 2022 •
edited
Loading