Fix GH-21682: Add NOT_SERIALIZABLE to XMLWriter, XMLReader, SNMP, tidy, tidyNode

[iliaal]

Part of #21682

Five classes wrap native C handles but allow serialization. Unserializing them produces objects with NULL internal pointers.

Segfault:

• tidyNode (ext/tidy): crashes on hasChildren(), dangling TidyNode pointer

Silent data loss:

• tidy (ext/tidy): body() returns NULL, cleanRepair() no-ops
• SNMP (ext/snmp): methods run against a dead session without error

Throws on use:

• XMLWriter (ext/xmlwriter): methods throw "Invalid or uninitialized XMLWriter object"
• XMLReader (ext/xmlreader): read() throws "Data must be loaded before reading"

Adds @not-serializable to all five so serialize() throws instead of producing broken objects.

The UAF exploit test bug72479.phpt instantiated SNMP via unserialize(). With NOT_SERIALIZABLE, unserialize() rejects the class and closes the UAF vector.

ZipArchive moved to a follow-up PR after review feedback.

[ndossche]

One thing to keep in mind is that it may introduce issues analogous to #8996
Perhaps the not-serializable flag is not the right way to do it, I was never really convinced that also blocking children is the right thing to do.

[iliaal]

Serializing these classes today: tidyNode segfaults on hasChildren(), ZipArchive/tidy silently lose state, XMLWriter/XMLReader throw on every method call. None produce a usable object.

Re child classes and GH-8996: only tidyNode is final, but unlike DOM none of these classes can capture or restore their internal C state. DOMDocument has saveXML()/loadXML(), which makes SerializableDomDocument viable. These classes have no equivalent. The underlying handles (xmlTextWriterPtr, TidyDoc, libzip archive, SNMP session) are opaque. A child adding __serialize would track its own state and rebuild from scratch on wakeup, not serialize the parent. The GH-8996 pattern doesn't apply here in practice.

Happy to switch to the DOM-style custom handler if you'd prefer that over NOT_SERIALIZABLE, I do think though it introduces complexity for little meaningful value though.

[ndossche]

Fair enough, see my nitpick remark.

[ndossche]

This test should be moved to ext/zip/tests

[iliaal]

fair enough, moved as asked.

[DanielEScherzer]

Re child classes and #8996: only tidyNode is final, but unlike DOM none of these classes can capture or restore their internal C state

ZipArchive will have a way to capture this soon with #21497, see https://github.com/tstarling/php-src/blob/de15d91b44fe56d7e02c4ada8e6e7c0ea13ef146/ext/zip/tests/ZipArchive_closeString_basic.phpt, so allowing subclasses to be serialized might be useful - can you split out the ZipArchive changes? In the absence of a maintainer for ext/zip we should at least give more consideration after #21497 before deciding to prevent subclass serialization

[iliaal]

Re child classes and #8996: only tidyNode is final, but unlike DOM none of these classes can capture or restore their internal C state

ZipArchive will have a way to capture this soon with #21497, see https://github.com/tstarling/php-src/blob/de15d91b44fe56d7e02c4ada8e6e7c0ea13ef146/ext/zip/tests/ZipArchive_closeString_basic.phpt, so allowing subclasses to be serialized might be useful - can you split out the ZipArchive changes? In the absence of a maintainer for ext/zip we should at least give more consideration after #21497 before deciding to prevent subclass serialization

Granted, closeString() makes subclass serialization workable. My reservation is narrower: the common case is the archive bytes, and openString()/closeString() already round-trip those without a wrapper. A subclass only helps when it carries extra state alongside the archive, which is a narrow niche. That said, I take the point that blocking it now forecloses a path that might matter later.

I'll split the ZipArchive change into its own PR so the other changes can go in. We can revisit Zip once #21497 merges and we see what subclasses actually need.

PR title needs to be updated, but otherwise no remaining objections about ZipArchive

[iliaal]

@DanielEScherzer Updated title, mind giving this a 2nd look plz

[DanielEScherzer]

@DanielEScherzer Updated title, mind giving this a 2nd look plz

No remaining objections from me, but I'm not sure if we w to do this generally, it is kind of a breaking change since it also restricts subclasses. Leaving it up to the individual extension maintainers to approve as usual

[Girgias]

It might make sense to see if we can make some of those classes final.

[iluuu1994]

Makes sense to me, thanks!

[TimWolla]

Suggested change

	echo $e->getMessage() . "\n";
	echo $e::class, ": ", $e->getMessage(), PHP_EOL;

This style is usually used nowadays to also assert the exception type, particularly since the introduction of the Throwable policy: https://github.com/php/policies/blob/main/coding-standards-and-naming.rst#throwables

Applies to all tests.

[iliaal]

Applied across all five tests; the expected output now asserts the Exception type too.