Comments (17)
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 includedself.SkipTest
commands, which are even applied rather inconsistently (as this issue shows)? Shouldn’t all tests includingSetReparseDeferralEnabled
andGetReparseDeferralEnabled
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 inlibexpat
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".
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:
- This was a
./configure --with-system-expat
build. (not our default build mode) - 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
andGetReparseDeferralEnabled
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 inlibexpat
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.
Related Issues (20)
- a biit better example in pathlib docs - maybe include example path that involves file in a folder(s)? HOT 1
- Security branches: Consider fixing the documentation or adding a notice banner HOT 1
- New REPL omits local variables when running ./python -i script.py HOT 2
- importlib.metadata test fixtures should prefer test.support fixtures
- Lacking description of how to correctly implement indexing syntax support.
- Rewrite asyncio subprocesses without child watchers
- Unpickling Exceptions with keyword arguments in multiprocessing throws an error HOT 12
- Reference leak in `_contextvars.Context.run()`
- Allow one to use build.bat to skip building test project files entirely. HOT 4
- CANT FIND THE FUCKING DOWNLOAD BUTTON HOT 2
- Typo in the documentation of the `cmd` parameter of `ftplib.FTP.retrbinary()` HOT 1
- ios buildbot failure: `enclose 'sqlite3_create_window_function' in a __builtin_available check to silence this warning` HOT 1
- generator frame type should not be PyObject*[]
- `subprocess.run` docs should recommend copying `os.environ` on Windows HOT 3
- `faulthandler` itself crashes in free-threading build (in `_Py_DumpExtensionModules`)
- Some Runtime Finalization Constraints Are Not Enforced Nor Documented
- asyncio REPL fails to run with TERM=dumb or PYTHON_BASIC_REPL in 3.13.0b2 HOT 5
- `__module__` is not defined, seeming to contradict the Python Data Model. HOT 13
- `PyDict_Next` should not lock the dict
- `type_setattro` error return paths contain bugs
Recommend Projects
-
React
A declarative, efficient, and flexible JavaScript library for building user interfaces.
-
Vue.js
🖖 Vue.js is a progressive, incrementally-adoptable JavaScript framework for building UI on the web.
-
Typescript
TypeScript is a superset of JavaScript that compiles to clean JavaScript output.
-
TensorFlow
An Open Source Machine Learning Framework for Everyone
-
Django
The Web framework for perfectionists with deadlines.
-
Laravel
A PHP framework for web artisans
-
D3
Bring data to life with SVG, Canvas and HTML. 📊📈🎉
-
Recommend Topics
-
javascript
JavaScript (JS) is a lightweight interpreted programming language with first-class functions.
-
web
Some thing interesting about web. New door for the world.
-
server
A server is a program made to process requests and deliver data to clients.
-
Machine learning
Machine learning is a way of modeling and interpreting data that allows a piece of software to respond intelligently.
-
Visualization
Some thing interesting about visualization, use data art
-
Game
Some thing interesting about game, make everyone happy.
Recommend Org
-
Facebook
We are working to build community through open source technology. NB: members must have two-factor auth.
-
Microsoft
Open source projects and samples from Microsoft.
-
Google
Google ❤️ Open Source for everyone.
-
Alibaba
Alibaba Open Source for everyone
-
D3
Data-Driven Documents codes.
-
Tencent
China tencent open source team.
from cpython.