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

serhiy-storchaka · 2022-11-20T07:56:18Z

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.

vstinner · 2022-11-21T09:17:57Z

Lib/test/test_capi/test_unicode.py

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


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.

vstinner · 2022-11-21T09:18:46Z

Lib/test/test_capi/test_unicode.py

@@ -11,6 +11,53 @@

 class CAPITest(unittest.TestCase):

+    @support.cpython_only


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?

vstinner · 2022-11-21T13:51:08Z

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.

vstinner

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.

serhiy-storchaka · 2022-11-21T15:54:38Z

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.

vstinner · 2022-11-21T16:39:30Z

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

vstinner · 2022-11-29T16:15:28Z

Lib/test/test_capi/test_codecs.py

+
+class CAPITest(unittest.TestCase):
+
+    @support.cpython_only


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.

vstinner · 2022-11-29T16:15:41Z

Lib/test/test_capi/test_codecs.py

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


IMO you can move it on the whole class.

vstinner · 2022-11-29T16:16:57Z

Lib/test/test_capi/test_codecs.py

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


Why not skipping this test if the module is missing?

vstinner · 2022-11-29T16:17:40Z

Lib/test/test_capi/test_codecs.py

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


You may use decodeutf8 = _testcapi.unicode_decodeutf8.

vstinner · 2022-11-29T16:18:48Z

Modules/_testcapi/unicode.c

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


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.

vstinner

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

serhiy-storchaka · 2022-12-01T12:53:39Z

I completely agree with you.

miss-islington · 2022-12-01T12:54:54Z

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

miss-islington · 2022-12-01T12:55:14Z

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

miss-islington · 2022-12-01T12:55:16Z

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>

bedevere-bot · 2023-07-25T10:06:33Z

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)

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

serhiy-storchaka commented Nov 20, 2022 •

edited by bedevere-bot

Loading

vstinner Nov 21, 2022

serhiy-storchaka Nov 21, 2022

serhiy-storchaka Nov 21, 2022

vstinner Nov 21, 2022

serhiy-storchaka Nov 21, 2022

vstinner commented Nov 21, 2022

vstinner left a comment

serhiy-storchaka commented Nov 21, 2022

vstinner commented Nov 21, 2022

vstinner Nov 29, 2022

vstinner Nov 29, 2022

vstinner Nov 29, 2022

vstinner Nov 29, 2022

vstinner Nov 29, 2022

vstinner left a comment

serhiy-storchaka commented Dec 1, 2022 •

edited

Loading

miss-islington commented Dec 1, 2022

miss-islington commented Dec 1, 2022

miss-islington commented Dec 1, 2022

bedevere-bot commented Jul 25, 2023

		@@ -11,6 +11,53 @@

		class CAPITest(unittest.TestCase):

		@support.cpython_only

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

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

Conversation

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vstinner commented Nov 21, 2022

vstinner left a comment

Choose a reason for hiding this comment

serhiy-storchaka commented Nov 21, 2022

vstinner commented Nov 21, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vstinner left a comment

Choose a reason for hiding this comment

serhiy-storchaka commented Dec 1, 2022 • edited Loading

miss-islington commented Dec 1, 2022

miss-islington commented Dec 1, 2022

miss-islington commented Dec 1, 2022

bedevere-bot commented Jul 25, 2023

serhiy-storchaka commented Nov 20, 2022 •

edited by bedevere-bot

Loading

serhiy-storchaka commented Dec 1, 2022 •

edited

Loading