gh-133546: Make `re.Match` a well-rounded `Sequence` type #133549

vberlier · 2025-05-07T02:55:53Z

Issue: Make re.Match a well-rounded Sequence typeÂ #133546

ðŸ“š Documentation preview ðŸ“š: https://cpython-previews--133549.org.readthedocs.build/

JelleZijlstra · 2025-05-07T03:59:33Z

Doc/library/re.rst

@@ -1378,6 +1378,27 @@ when there is no match, you can test whether there was a match with a simple
   if match:
       process(match)

+Match objects are proper :class:`~collections.abc.Sequence` types. You can access


This is not true with this PR, Sequence has a number of other requirements (e.g. an index and count method).

Nice catch, thanks! I added index and count by adapting the implementation of tuple.index and tuple.count. I also updated the unit test to cover all Sequence mixin methods.

ZeroIntensity · 2025-05-11T16:06:27Z

Modules/_sre/sre.c

+
+    if (start < 0) {
+        start += self->groups;
+        if (start < 0)


You should take a quick look at PEP-7 for our style guide. We try to avoid omitting brackets. There is a bit of inconsistency in this file, but it's good to just follow PEP 7 for new code.

I read through PEP-7 already but I didn't really think about it here since I copied this directly from tupleobject.c. Let me clean this up!

Modules/_sre/sre.c

ZeroIntensity · 2025-05-11T16:13:58Z

Modules/_sre/sre.c

+
+    if (index < 0 || index >= self->groups) {
+        /* raise IndexError if we were given a bad group number */
+        if (!PyErr_Occurred()) {


We shouldn't conditionally raise the error. match_item shouldn't ever be called with an exception set.

ZeroIntensity · 2025-05-11T16:18:09Z

Modules/_sre/sre.c

+        PyObject* group = match_getslice_by_index(self, i, Py_None);
+        if (group == NULL)
+            return NULL;
+        int cmp = PyObject_RichCompareBool(group, value, Py_EQ);


I'm pretty sure we need to incref the value in case the __bool__ releases a reference. cc @picnixz who fixed a bunch of issues related to that.

Not sure if this is what you're talking about, but I think this is already handled by PyObject_RichCompareBool out of the box. It's a wrapper around PyObject_RichCompare that unwraps and decref the returned object.

It's not about the returned object, it's about the inputs. PyObject_RichCompareBool and PyObject_RichCompare can call arbitrary Python code in general, so if such arbitrary code actually releases or changes their inputs, we may end up with what we call a "use-after-free" (and usually it leads to a SIGSEV, but in other occurrences it can be a security issue).

In order to determine whether there is a UAF or not, you need to check whether group or value can actually be obtained from pure Python code and be changed by the call to __eq__. In this case, value can be modified in place by its call, but _sre_SRE_Match_index_impl should have already held a strong reference on it, so there shouldn't be a UAF.

I see, thanks for the explanation!

picnixz

Test coverage is sometimes insufficient and sometimes redundant with existing ones.

picnixz · 2025-05-15T12:22:48Z

Doc/library/re.rst

@@ -1473,6 +1494,37 @@ when there is no match, you can test whether there was a match with a simple

   .. versionadded:: 3.6

+   .. versionchanged:: 3.14


Suggested change

.. versionchanged:: 3.14

.. versionchanged:: next

picnixz · 2025-05-15T12:23:08Z

Doc/library/re.rst

+      >>> len(m)
+      3
+
+   .. versionadded:: 3.14


Suggested change

.. versionadded:: 3.14

.. versionadded:: next

picnixz · 2025-05-15T12:23:59Z

Doc/library/re.rst

+
+   Raises :exc:`ValueError` if the value is not present.
+
+   .. versionadded:: 3.14


Suggested change

.. versionadded:: 3.14

.. versionadded:: next

picnixz · 2025-05-15T12:24:04Z

Doc/library/re.rst

+
+   Return the number of occurrences of the value among the matched groups.
+
+   .. versionadded:: 3.14


Suggested change

.. versionadded:: 3.14

.. versionadded:: next

Doc/library/re.rst

picnixz · 2025-05-15T13:45:52Z

Lib/test/test_re.py

+        self.assertEqual(m[1:-1], ("a", "b"))
+        self.assertEqual(m[::-1], ("c", "b", "a", "abc"))


Let's check more slices.

picnixz · 2025-05-15T13:46:15Z

Lib/test/test_re.py

+        self.assertEqual(m.count("a"), 1)
+        self.assertEqual(m.count("b"), 1)
+        self.assertEqual(m.count("c"), 1)
+        self.assertEqual(m.count("123"), 0)


Let's check with non-string objects.

picnixz · 2025-05-15T13:46:55Z

Lib/test/test_re.py

+            case [_, "a", "b", "c"]:
+                pass


Let's catch the first group (the entire match) and assert it.

picnixz · 2025-05-15T13:47:07Z

Lib/test/test_re.py

+                self.fail()
+
+        match re.match(r"(\d+)-(\d+)-(\d+)", "2025-05-07"):
+            case [_, year, month, day]:


Let's do the same here.

picnixz · 2025-05-15T13:48:09Z

Lib/test/test_re.py

@@ -599,8 +603,83 @@ def test_match_getitem(self):
        with self.assertRaises(TypeError):
            m[0] = 1

-        # No len().
-        self.assertRaises(TypeError, len, m)
+    def test_match_sequence(self):


Let's split this into multiple test: _index, _count and _iter, as well as match case. For getitem, let's move them under test_match_getitem as well and reduce the number of duplicated checks.

picnixz

Some additional feedback. For the test failures, let's add some assertWarns. I don't think we should make some additional checks in C for the type of the operands (the issue is when you compare the input (which is a bytes) with the matched item (which are strings), hence a BytesWarning). So, instead, let's just expect the warning.

By the way, could you avoid using @picnixz in your commits as I'm getting pinged? TiA.

picnixz · 2025-05-15T23:48:17Z

Lib/test/test_re.py

-    def test_match_sequence(self):
-        from collections.abc import Sequence
-
+        # Slices.


Slices can be part of a new test if you want.

picnixz · 2025-05-15T23:49:25Z

Modules/_sre/sre.c

@@ -2491,7 +2491,10 @@ match_subscript(PyObject *op, PyObject* item)
        if (self->pattern->groupindex) {
            PyObject* index = PyDict_GetItemWithError(self->pattern->groupindex, item);
            if (index && PyLong_Check(index)) {
-                return match_item(op, PyLong_AsSsize_t(index));
+                Py_ssize_t i = PyLong_AsSsize_t(index);
+                if (i != -1) {


-1 is strictly speaking a valid one, so you should use PyErr_Occurred()

picnixz · 2025-05-15T23:49:50Z

Modules/_sre/sre.c

@@ -2750,12 +2741,12 @@ _sre_SRE_Match_count_impl(MatchObject *self, PyObject *value)
        }
        int cmp = PyObject_RichCompareBool(group, value, Py_EQ);
        Py_DECREF(group);
+        if (cmp < 0) {
+            return NULL;
+        }
        if (cmp > 0) {


Now you can use an else if here

vberlier · 2025-05-16T00:19:30Z

Oh, sorry for the ping. Is it okay if I just leave out the cases with bytes as input? I can't seem to trigger the warning locally, even with ./python -Werror -m test.

bedevere-app bot mentioned this pull request May 7, 2025

Make re.Match a well-rounded Sequence type #133546

Open

bedevere-app bot added the awaiting review label May 7, 2025

Make re.Match a well-rounded Sequence type

74480a7

vberlier force-pushed the gh-133546 branch from ac95ba2 to 74480a7 Compare May 7, 2025 03:02

JelleZijlstra reviewed May 7, 2025

View reviewed changes

vberlier force-pushed the gh-133546 branch from 01288ce to 497c42a Compare May 7, 2025 05:23

Implement missing index and count methods

a3de846

vberlier force-pushed the gh-133546 branch from 497c42a to a3de846 Compare May 8, 2025 11:08

ZeroIntensity reviewed May 11, 2025

View reviewed changes

vberlier added 6 commits May 11, 2025 20:15

Don't check for PyErr_Occurred

603b1d1

Add missing braces

70b73e4

Fix link to ValueError

f51ef45

Rewrite match_subscript to support negative indexing and slicing

f218828

Update ACKS

5272141

Add news entry

d0aa6fa

picnixz reviewed May 15, 2025

View reviewed changes

Address feedback from @picnixz

5f67be0

vberlier force-pushed the gh-133546 branch from ccdbbbb to 5f67be0 Compare May 15, 2025 23:57

vberlier added 3 commits May 16, 2025 02:21

Move slicing test cases into its own test function

17feaa6

Fix error checking

fe709f8

Use else if

4095b52

vberlier requested a review from picnixz May 20, 2025 17:29

Apr	MAY	Jun
	22
2024	2025	2026

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

gh-133546: Make `re.Match` a well-rounded `Sequence` type #133549

gh-133546: Make `re.Match` a well-rounded `Sequence` type #133549

vberlier commented May 7, 2025 •

edited by github-actions bot

Loading

JelleZijlstra May 7, 2025

vberlier May 7, 2025

ZeroIntensity May 11, 2025

vberlier May 11, 2025

ZeroIntensity May 11, 2025

vberlier May 11, 2025

ZeroIntensity May 11, 2025

vberlier May 11, 2025

picnixz May 15, 2025 •

edited

Loading

vberlier May 15, 2025

picnixz left a comment

picnixz May 15, 2025

picnixz May 15, 2025

picnixz May 15, 2025

picnixz May 15, 2025

picnixz May 15, 2025

picnixz May 15, 2025

picnixz May 15, 2025

picnixz May 15, 2025

picnixz May 15, 2025

picnixz left a comment

picnixz May 15, 2025

picnixz May 15, 2025

picnixz May 15, 2025

vberlier commented May 16, 2025

		@@ -1473,6 +1494,37 @@ when there is no match, you can test whether there was a match with a simple

		.. versionadded:: 3.6

		.. versionchanged:: 3.14


		Raises :exc:`ValueError` if the value is not present.

		.. versionadded:: 3.14


		Return the number of occurrences of the value among the matched groups.

		.. versionadded:: 3.14

		self.assertEqual(m[1:-1], ("a", "b"))
		self.assertEqual(m[::-1], ("c", "b", "a", "abc"))

gh-133546: Make re.Match a well-rounded Sequence type #133549

Are you sure you want to change the base?

gh-133546: Make re.Match a well-rounded Sequence type #133549

Conversation

vberlier commented May 7, 2025 • edited by github-actions 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

picnixz May 15, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

picnixz left a 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

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

Choose a reason for hiding this comment

picnixz left a 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

vberlier commented May 16, 2025

gh-133546: Make `re.Match` a well-rounded `Sequence` type #133549

gh-133546: Make `re.Match` a well-rounded `Sequence` type #133549

vberlier commented May 7, 2025 •

edited by github-actions bot

Loading

picnixz May 15, 2025 •

edited

Loading