Both test_flush_reparse_deferral_disabled tests fail with older libexpat (2.4.4) about cpython HOT 17 CLOSED

breaks with the old libexpat (of course, patched for the security issues):

@mcepl that's expected and distros backporting libexpat security fixed need to adjust the tests (and C pre-processor checks!) of CPython at the same time if doing so. This is a downstream issue, not an upstream one. I'm not alone in this view, e.g. see #115623 (review) .

I am rather confused by the coding style of #115623 … isn’t there a preference for @unittest.skipIf instead of randomly included self.SkipTest commands, which are even applied rather inconsistently (as this issue shows)? Shouldn’t all tests including SetReparseDeferralEnabled and GetReparseDeferralEnabled be just covered by @unittest.skipIf(pyexpat.version_info < (2, 6, 0)) and be done with it, if those methods (as mentioned in the documentation) were introduced just in libexpat 2.6.0?

Wouldn’t something like this be more appropriate?

@mcepl hunk 1 and 3 are purely cosmetic, and hence of limited interest to me:

Hunk 1:

---
 Lib/test/test_sax.py       |   10 +++++-----
 Lib/test/test_xml_etree.py |   17 ++++++++---------
 2 files changed, 13 insertions(+), 14 deletions(-)

--- a/Lib/test/test_sax.py
+++ b/Lib/test/test_sax.py
@@ -1211,10 +1211,9 @@ class ExpatReaderTest(XmlTestBase):
 
         self.assertEqual(result.getvalue(), start + b"<doc>text</doc>")
 
+    @unittest.skipIf(pyexpat.version_info < (2, 6, 0),
+                     "Reparse deferral not defined for libexpat < 2.6.0")
     def test_flush_reparse_deferral_enabled(self):
-        if pyexpat.version_info < (2, 6, 0):
-            self.skipTest(f'Expat {pyexpat.version_info} does not support reparse deferral')
-
         result = BytesIO()
         xmlgen = XMLGenerator(result)
         parser = create_parser()

Hunk 3:

--- a/Lib/test/test_xml_etree.py
+++ b/Lib/test/test_xml_etree.py
@@ -1619,11 +1619,9 @@ class XMLPullParserTest(unittest.TestCas
         with self.assertRaises(ValueError):
             ET.XMLPullParser(events=('start', 'end', 'bogus'))
 
+    @unittest.skipIf(pyexpat.version_info < (2, 6, 0),
+                     "Reparse deferral not defined for libexpat < 2.6.0")
     def test_flush_reparse_deferral_enabled(self):
-        if pyexpat.version_info < (2, 6, 0):
-            self.skipTest(f'Expat {pyexpat.version_info} does not '
-                          'support reparse deferral')
-
         parser = ET.XMLPullParser(events=('start', 'end'))
 
         for chunk in ("<doc", ">"):

Hunk 2 and 4 are a very different story:

Hunk 2:

@@ -1236,6 +1235,8 @@ class ExpatReaderTest(XmlTestBase):
 
         self.assertEqual(result.getvalue(), start + b"<doc></doc>")
 
+    @unittest.skipIf(pyexpat.version_info < (2, 6, 0),
+                     "Reparse deferral not defined for libexpat < 2.6.0")
     def test_flush_reparse_deferral_disabled(self):
         result = BytesIO()
         xmlgen = XMLGenerator(result)
@@ -1245,8 +1246,7 @@ class ExpatReaderTest(XmlTestBase):
         for chunk in ("<doc", ">"):
             parser.feed(chunk)
 
-        if pyexpat.version_info >= (2, 6, 0):
-            parser._parser.SetReparseDeferralEnabled(False)
+        parser._parser.SetReparseDeferralEnabled(False)
 
         self.assertEqual(result.getvalue(), start)  # i.e. no elements started
         self.assertFalse(parser._parser.GetReparseDeferralEnabled())

Hunk 4:

@@ -1644,17 +1642,18 @@ class XMLPullParserTest(unittest.TestCas
 
         self.assert_event_tags(parser, [('end', 'doc')])
 
+    @unittest.skipIf(pyexpat.version_info < (2, 6, 0),
+                     "Reparse deferral not defined for libexpat < 2.6.0")
     def test_flush_reparse_deferral_disabled(self):
         parser = ET.XMLPullParser(events=('start', 'end'))
 
         for chunk in ("<doc", ">"):
             parser.feed(chunk)
 
-        if pyexpat.version_info >= (2, 6, 0):
-            if not ET is pyET:
-                self.skipTest(f'XMLParser.(Get|Set)ReparseDeferralEnabled '
-                              'methods not available in C')
-            parser._parser._parser.SetReparseDeferralEnabled(False)
+        if not ET is pyET:
+            self.skipTest(f'XMLParser.(Get|Set)ReparseDeferralEnabled '
+                          'methods not available in C')
+        parser._parser._parser.SetReparseDeferralEnabled(False)
 
         self.assert_event_tags(parser, [])  # i.e. no elements started
         if ET is pyET:

@mcepl both hunk 2 and 4 seem to change semantics for the worse, as tests that were previously running with <2.6.0 fine are now skipped without need to. This inconsistency was done on full purpose.

I'm not against simplifying the two of the four skip markers that keep semantics intact, but I don't consider it of much value, personally.

My vote for closing this issue as "works as expected, no upstream issue to fix".

CC @serhiy-storchaka @gpshead

from cpython.

Hi @mcepl,

@mcepl PS: Please note that these functions are exposed by CPython even with libexpat <2.6.0. They just don't do that much then.
So, you admit that the documentation is wrong.

I don't, and I don't see how it would be wrong. What does the documentation state that is incorrect and which documentation exactly?

Also, why then these tests fail with older libexpat?

Because of the security patches that distros backported into older releases of libexpat that got you into unsupported territory between old and new releases. The failing version is not vanilla 2.4.4 or it would not fail. It's 2.4.4 with backported patches.

from cpython.

I completely agree with @hartwork. Python tests work pretty well with both new and old versions of libexpat. They do not work with a distro-specific build of libexpat with distro-specific patches applied. This is not supported case. You perhaps need to apply a distro-specific patch for Python tests to make them consistent with the rest of of the distro. Or what is the standard way to solve such issues in your distro.

from cpython.

Hi, glad you two are chatting.

Looking at the build log you link to from the opening message, the important bit is:

1. This was a ./configure --with-system-expat build. (not our default build mode)
2. The system expat is version 2.4.4.

There were a pile of distro patches on top of that expat and Python applied, but none of those matter. (No backport of the related expat 2.6.0 security improvement feature was involved).

This is easy to reproduce on current non-bleeding-edge Debian and Ubuntu today (sporting expat 2.5.0 or earlier), make sure libexpat1-dev is installed, and presumbly a bunch of other distros (like opensuse or SuSE as was being used):

./configure --with-system-expat && ./python -m test test_sax test_xml_etree

Unfortunately, we do not test --with-system-expat in any way as part of the CPython project. So this failing right now isn't too surprising. That flag exists primarily for use by external distributors who unvendor libraries.

A good way to help prevent this kind of building against legacy libraries failure from happening in the future is to contribute & maintain a relevant builtbot. https://devguide.python.org/testing/new-buildbot-worker/

from cpython.

Python tests work pretty well with both new and old versions of libexpat.

This currently appears to unfortunately be false because we don't test this anywhere.

from cpython.

@gpshead is right, I can now reproduce the problem for vanilla Expat <2.6.0. Debian trixie has CVE-2023-52425 unfixed as of today, so we can use Debian trixie for a reproducer (which is great and also not great). Here's a reproducer Dockerfile:

FROM debian:trixie
RUN apt-get update \
        && \
    apt-get install --yes --no-install-recommends -V \
            ca-certificates \
            build-essential \
            git \
            libexpat1-dev \
            pkg-config
RUN git clone --depth 1 https://github.com/python/cpython
WORKDIR cpython
RUN git rev-parse HEAD
RUN ./configure --with-system-expat && make -j$(nproc)
RUN ./python -m test test_sax test_xml_etree

So full 180% turn, @mcepl did uncover a bug in the test suite here. Thanks for bringing this up!

I just created a pull request #117203 just now for a fix, and would welcome review and someone adding backport labels to it. Thanks in advance!

PS: To see the tests pass with the pull request, apply this patch to the Dockerfile above:

--- RUN git clone --depth 1 https://github.com/python/cpython
+++ RUN git clone --depth 1 --branch issue-117187-fix-xml-tests-for-vanilla-expat-2-5-0 https://github.com/hartwork/cpython

Have a good start to the week! 🍻

from cpython.

I am rather confused by the coding style of #115623 … isn’t there a preference for @unittest.skipIf instead of randomly included self.SkipTest commands, which are even applied rather inconsistently (as this issue shows)? Shouldn’t all tests including SetReparseDeferralEnabled and GetReparseDeferralEnabled be just covered by @unittest.skipIf(pyexpat.version_info < (2, 6, 0)) and be done with it, if those methods (as mentioned in the documentation) were introduced just in libexpat 2.6.0?

Wouldn’t something like this be more appropriate?

---
 Lib/test/test_sax.py       |   10 +++++-----
 Lib/test/test_xml_etree.py |   17 ++++++++---------
 2 files changed, 13 insertions(+), 14 deletions(-)

--- a/Lib/test/test_sax.py
+++ b/Lib/test/test_sax.py
@@ -1211,10 +1211,9 @@ class ExpatReaderTest(XmlTestBase):
 
         self.assertEqual(result.getvalue(), start + b"<doc>text</doc>")
 
+    @unittest.skipIf(pyexpat.version_info < (2, 6, 0),
+                     "Reparse deferral not defined for libexpat < 2.6.0")
     def test_flush_reparse_deferral_enabled(self):
-        if pyexpat.version_info < (2, 6, 0):
-            self.skipTest(f'Expat {pyexpat.version_info} does not support reparse deferral')
-
         result = BytesIO()
         xmlgen = XMLGenerator(result)
         parser = create_parser()
@@ -1236,6 +1235,8 @@ class ExpatReaderTest(XmlTestBase):
 
         self.assertEqual(result.getvalue(), start + b"<doc></doc>")
 
+    @unittest.skipIf(pyexpat.version_info < (2, 6, 0),
+                     "Reparse deferral not defined for libexpat < 2.6.0")
     def test_flush_reparse_deferral_disabled(self):
         result = BytesIO()
         xmlgen = XMLGenerator(result)
@@ -1245,8 +1246,7 @@ class ExpatReaderTest(XmlTestBase):
         for chunk in ("<doc", ">"):
             parser.feed(chunk)
 
-        if pyexpat.version_info >= (2, 6, 0):
-            parser._parser.SetReparseDeferralEnabled(False)
+        parser._parser.SetReparseDeferralEnabled(False)
 
         self.assertEqual(result.getvalue(), start)  # i.e. no elements started
         self.assertFalse(parser._parser.GetReparseDeferralEnabled())
--- a/Lib/test/test_xml_etree.py
+++ b/Lib/test/test_xml_etree.py
@@ -1619,11 +1619,9 @@ class XMLPullParserTest(unittest.TestCas
         with self.assertRaises(ValueError):
             ET.XMLPullParser(events=('start', 'end', 'bogus'))
 
+    @unittest.skipIf(pyexpat.version_info < (2, 6, 0),
+                     "Reparse deferral not defined for libexpat < 2.6.0")
     def test_flush_reparse_deferral_enabled(self):
-        if pyexpat.version_info < (2, 6, 0):
-            self.skipTest(f'Expat {pyexpat.version_info} does not '
-                          'support reparse deferral')
-
         parser = ET.XMLPullParser(events=('start', 'end'))
 
         for chunk in ("<doc", ">"):
@@ -1644,17 +1642,18 @@ class XMLPullParserTest(unittest.TestCas
 
         self.assert_event_tags(parser, [('end', 'doc')])
 
+    @unittest.skipIf(pyexpat.version_info < (2, 6, 0),
+                     "Reparse deferral not defined for libexpat < 2.6.0")
     def test_flush_reparse_deferral_disabled(self):
         parser = ET.XMLPullParser(events=('start', 'end'))
 
         for chunk in ("<doc", ">"):
             parser.feed(chunk)
 
-        if pyexpat.version_info >= (2, 6, 0):
-            if not ET is pyET:
-                self.skipTest(f'XMLParser.(Get|Set)ReparseDeferralEnabled '
-                              'methods not available in C')
-            parser._parser._parser.SetReparseDeferralEnabled(False)
+        if not ET is pyET:
+            self.skipTest(f'XMLParser.(Get|Set)ReparseDeferralEnabled '
+                          'methods not available in C')
+        parser._parser._parser.SetReparseDeferralEnabled(False)
 
         self.assert_event_tags(parser, [])  # i.e. no elements started
         if ET is pyET:

from cpython.

cc @hartwork

from cpython.

Shouldn’t all tests including SetReparseDeferralEnabled and GetReparseDeferralEnabled be just covered by @unittest.skipIf(pyexpat.version_info < (2, 6, 0)) and be done with it, if those methods (as mentioned in the documentation) were introduced just in libexpat 2.6.0?

@mcepl PS: Please note that these functions are exposed by CPython even with libexpat <2.6.0. They just don't do that much then.

from cpython.

from cpython.

from cpython.

@mcepl I have sent you an e-mail offering a voice call in the mean time, about 30 minutes ago. I will refrain from replying here more for now, before we had a real chance to talk. Thanks for your consideration.

from cpython.

@mcepl I have sent you an e-mail offering a voice call in the mean time, about 30 minutes ago. I will refrain from replying here more for now, before we had a real chance to talk. Thanks for your consideration.

I am in the Central European Time zone, and it is Sunday night here. As I see you are in Berlin, we can talk tomorrow (mcepl on Libera.chat (preferable), or @mcepl:one.ems.host on Matrix).

from cpython.

@mcepl Matrix and IRC do not work for me, but we'll find something else. Let's take call org to e-mail to keep the issue on topic. See you on Monday, have a good start to the week.

from cpython.

@gpshead I'm investigating your statement now to prove it right or wrong, not on fast hardware right now, will be back with results in <45 minutes I hope.

from cpython.

Thanks! main merged, 3.12 and 3.11 set to automerge, and PRs assigned to a RM with releaseblocker status for the security only branches that received the expat security fix leading to this issue.

No decision has been made on whether to try and handle the "expat with local backported behavior changes" case that causes test suite failures as found with Ubuntu's expat_2.5.0-2ubuntu0.1 package. Ideally a distro needing that would contribute that?

from cpython.

Thanks! main merged, 3.12 and 3.11 set to automerge, and PRs assigned to a RM with releaseblocker status for the security only branches that received the expat security fix leading to this issue.

@gpshead thank you!

No decision has been made on whether to try and handle the "expat with local backported behavior changes" case that causes test suite failures as found with Ubuntu's expat_2.5.0-2ubuntu0.1 package. Ideally a distro needing that would contribute that?

I would argue that support for downstream backports would be out of place upstream. Supporting multiple versions of vanilla Expat is complex enough. So instead of asking for these patches for upstream, let's not patch upstream and keep the whole backporting cake downstream.

from cpython.