Fix tests for libexpat >=2.6.0 (or with CVE-2023-52425 fixes backported)

[hartwork]

Hi! 👋

It was reported that the clustershell test suite breaks with Expat >=2.6.0 (https://bugs.gentoo.org/924601). So this pull request fixes the test suite for Expat >=2.6.0 with CPython 3.13 and backport releases by using the new pyexpat API (python/cpython#115398). There is no rush in merging this, we can definitey wait for that APi to become more widely available if you like, it will likely appear in these very CPython releases:

• 3.8.19
• 3.9.19
• 3.10.14
• 3.11.9
• 3.12.3
• 3.13.0

I should note that Python 3.7 and earlier are dead, hence related pull request #555.
I'm looking forward to your review, maybe I missed something 🍻

Best, Sebastian

[hartwork]

Quick update on CPython: All related backport pull requests have now been merged. The next step is CPython security releases and then distros picking them up.

Regarding related expected releases dates, so far I only found:

• 3.11.9: Monday, 2024-04-01 — https://peps.python.org/pep-0664/
• 3.12.3: Tuesday, 2024-04-09 — https://peps.python.org/pep-0693/
• 3.13.0 final: Tuesday, 2024-10-01 — https://peps.python.org/pep-0719/

[hartwork]

@thiell by any chance do you know if the issue I'm fixing for the tests here affects the core ClusterShell software also? Is it just the tests that break with Expat >=2.6.0 or more?

[hartwork]

• 3.8.19
• 3.9.19
• 3.10.14

PS: These three are out since yesterday March 19, 2024:

• 3.8.19 — https://www.python.org/downloads/release/python-3819/
• 3.9.19 — https://www.python.org/downloads/release/python-3919/
• 3.10.14 — https://www.python.org/downloads/release/python-31014/

[hartwork]

@thiell by any chance do you know if the issue I'm fixing for the tests here affects the core ClusterShell software also? Is it just the tests that break with Expat >=2.6.0 or more?

@thiell any chance for a reply this week? We'll have to make decision in Gentoo whether to stabilize libexpat >=2.6.0 or not for all users, and knowing if core ClusterShell is affected outside the tests is the last missing piece in the picture, before making a decision. Thanks in advance, have a good start to the week 🍻

[thiell]

@hartwork When ClusterShell is configured to use the tree execution mode, the communication protocol with the remote ClusterShell gateway(s) uses XML and we parse it incrementally with an xml.sax.xmlreader.IncrementalParser. So the only instance of IncrementalParser.feed() in the core ClusterShell code is done here: https://github.com/cea-hpc/clustershell/blob/master/lib/ClusterShell/Communication.py#L231

A question I have is does the issue only occur with multithreaded codes? The test case uses two threads for example to simulate a remote gateway. In Communication.py, unlike the test in tests/TreeGatewayTest.py, a single thread is used and the producer of the XML content is remote, on a different machine.

[hartwork]

@thiell thanks for your detailed reply! 👍

The way I read the linked code, self._xml_reader is only used as an event FIFO that is emptied at the speed of availability at:

clustershell/lib/ClusterShell/Communication.py

Lines 254 to 258 in ec8be76

	# pass messages to the driver if ready
	while self._xml_reader.msg_available():
	msg = self._xml_reader.pop_msg()
	assert msg is not None
	self.recv(msg)

To me, it looks like the parser can just have its pace and the receivers need to be okay with any rate. If that's reality, it probably means that there is no problem with an occasional delay in events?

With regard to multi-threading, my understanding is that it's technically not the two threads but their blocking lockstep relationship that breaks with Expat >=2.6.0: every parser feed was expected to produce events immediately by the test code. The fix to CVE-2023-52425 in Expat can add delay to creation of events — firing of handlers in the language of Expat — depending on how big or small input chunks get, and that made one thread wait for an event that would not be produced without further input.

Maybe a good stress test would be to turn…

clustershell/lib/ClusterShell/Communication.py

Line 231 in ec8be76

self._parser.feed(msg + b'\n')

…into a loop feeding single bytes to make things worst (and be reverted), and running that with Expat >=2.6.0, if someone gets a chance to.

What do you think?

PS: On a side note Expat is not thread-safe and multi-threaded access would need synchronization around it, likely even from Python code (python/cpython#62170), not just in C. (I'm in favor of considering that a bug in CPython.)

[hartwork]

• 3.11.9: Monday, 2024-04-01 — https://peps.python.org/pep-0664/

Released by now on 2024-04-02 — https://docs.python.org/release/3.11.9/whatsnew/changelog.html#python-3-11-9

• 3.12.3: Tuesday, 2024-04-09 — https://peps.python.org/pep-0693/

Released by now on 2024-04-09 — https://docs.python.org/release/3.12.3/whatsnew/changelog.html#python-3-12-2

[hartwork]

@thiell I would expect CI on master to fail by now because it has 2.4.7-1ubuntu0.3 installed for all but Python 3.6 and that comes with the CVE-2023-52425 fixes according to https://changelogs.ubuntu.com/changelogs/pool/main/e/expat/expat_2.4.7-1ubuntu0.3/changelog but for some reason it's no failing — any ideas why? And: How do you feel about this pull request today?

[thiell]

PS: On a side note Expat is not thread-safe and multi-threaded access would need synchronization around it, likely even from Python code (python/cpython#62170), not just in C. (I'm in favor of considering that a bug in CPython.)

Thanks for the clarification. That's fine in our case as we communicate through pipes.

@thiell I would expect CI on master to fail by now because it has 2.4.7-1ubuntu0.3 installed for all but Python 3.6 and that comes with the CVE-2023-52425 fixes according to https://changelogs.ubuntu.com/changelogs/pool/main/e/expat/expat_2.4.7-1ubuntu0.3/changelog but for some reason it's no failing — any ideas why? And: How do you feel about this pull request today?

Actually, I noticed some random hung tests, that I relaunched manually. I just added support for testing with Python 3.12 too, and I saw a hung test. I think this is likely related to this issue.

I reviewed your proposed changes and I am ok to merge them as a first step. Note that I personally don't love the additional flush() method which is called after each feed() + need to be checked with hasattr(). I prefer the Get/SetReparseDeferralEnabled() API, which can restore the previous behavior.

Now apart from this test, I think we could have some problems with delayed events in the parser when used in clustershell's tree mode: XML is parsed incrementally between cluster nodes and the connection might be established for a long time between then, triggering events when they arrive. The source XML is generated by clustershell itself, so I think it would be fine in terms of security to just disable this reparse deferral feature with SetReparseDeferralEnabled(false) if available.

Sorry to be a bit slow but thanks for your help with this!

[thiell]

@hartwork Ah, interestingly, it looks like the incremental SAX parser that we use (ExpatParser) didn't get the same API than the xmlparser (xml.parsers.expat.xmlparser). With Python 3.12.5, I get the following error when trying to call ExpatParser.SetReparseDeferralEnabled():

  File "/home/runner/work/clustershell/clustershell/lib/ClusterShell/Communication.py", line 194, in __init__
    self._parser.SetReparseDeferralEnabled(False)
    ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
AttributeError: 'ExpatParser' object has no attribute 'SetReparseDeferralEnabled'

[hartwork]

@hartwork Ah, interestingly, it looks like the incremental SAX parser that we use (ExpatParser) didn't get the same API than the xmlparser (xml.parsers.expat.xmlparser).

@thiell correct, the description of python/cpython#115623 has the list of all related methods added (and to which class). The involved classes are quite different and there was not much room for how to implement things in CPython.

With Python 3.12.5, I get the following error when trying to call ExpatParser.SetReparseDeferralEnabled():

  File "/home/runner/work/clustershell/clustershell/lib/ClusterShell/Communication.py", line 194, in __init__
    self._parser.SetReparseDeferralEnabled(False)
    ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
AttributeError: 'ExpatParser' object has no attribute 'SetReparseDeferralEnabled'

Correct, there is no such method in that class.

Could you summarize status and plans for the new pull request #569? That would be great!