XMLParser fixes (SR-13546, SR-2301)

[dhoepfl]

XMLParser on linux was not reentrance safe.

While fixing this, I found that XMLParser also never calls several other delegate methods. (see SR-2301)

Still unfixed: Internal and unparsed external entities might not work as XML expects them to work. Unfortunately I could not find any reference (asides from the XML spec) how entities are actually expected to work.

macOS’s NSXMLParser does not seem to support them at all.

[MaxDesiatov]

Would you be able to clean up these commented lines or add more context around them please? When the final code is reviewed without any context of this PR, it won't be clear why they are commented out and what the implications of that are.

[dhoepfl]

Sure, will do so.

I sorted the fields as they are in _xmlSAXHandler and added all unused fields as comments to make it more obvious what is (not) used.
Do you think it is better to remove the unused fields or to add a short explanation above the block with “currently not used by CFXMLInterface” at these fields?

[MaxDesiatov]

@millenomi WDYT? What kind of clarification in code comments would you prefer?

[dhoepfl]

I just pushed a minor update that adds some comments.

[spevans]

@swift-ci test

[dhoepfl]

@swift-ci test

[spevans]

@swift-ci test

[spevans]

@swift-ci test

[dhoepfl]

Can you trigger it once again?

[MaxDesiatov]

@swift-ci please test

[dhoepfl]

I’m about to give up on this. 😟

[MaxDesiatov]

@swift-ci please test

[dhoepfl]

Does Build #2526 count for macOS?

If so, both platforms have a successful build now.

[MaxDesiatov]

@swift-ci please test

[dhoepfl]

“Swift Test macOS Platform” finished a build yesterday, another try?

[MaxDesiatov]

@swift-ci please test macOS platform

[spevans]

I ran the two testcases on Big Sur usingDarwinCompatibilityTests and they both failed.
test_sr13546_stackedParsers failed with NSXMLParser does not support reentrant parsing. (NSInternalConsistencyException) so it looks like Darwin Foundation explicitly blocks reentrancy
(The equivalent on Linux would actually be to call fatalError())

test_entities() also had test failures with the expected entities, the Darwin version doesnt emit the .foundIgnoreableWhitespace event and some of the declarations are missing entities. I suspect this could be due to different versions of libxml2 on Darwin v Linux - I have seen different events emitted in other tests when comparing Darwin to Linux.

I think for the first test re-entrancy will need to be blocked to match Darwin. I dont think there is much that can be done with the 2nd one and probably underlies libxml2 differences. It might be worth at some point building swift-corelibs-foundation against the version that Darwin uses (https://opensource.apple.com/source/libxml2/libxml2-34.9/) and seeing if it fixes any of these minor bugs (and in other tests that fail on Darwin as well)

[dhoepfl]

You are right, the implementation does not mimic Apple/Darwin’s implementation in all aspects.

I do not see how XMLParser could support (external) entities without being re-entrance-tolerant. Maybe that’s the reason Apple’s implementation does not support entities?

I consider entity handling in NSXMLParser broken:

let xml = """
<?xml version="1.0" encoding="ISO-8859-1"?>
<!DOCTYPE root [
<!ENTITY internalEntity "Internal">
<!ELEMENT root ANY >
]>
<root><foo>&internalEntity;</foo></root>
"""

class MyXMLParserDelegate: NSObject, XMLParserDelegate {
   func parser(_ parser: XMLParser, foundCharacters string: String) {
      print("characters: \(string)")
   }
}

let xmlData = xml.data(using: .utf8)!
let parser = XMLParser(data: xmlData)
let del = MyXMLParserDelegate()
parser.delegate = del
_ = parser.parse()

Never calls func parser(_ parser: XMLParser, foundCharacters string: String) on macOS, silently loosing the content of foo. The pull-version correctly prints:

characters: Internal

---

The difference in .foundIgnoreableWhitespace is easy to explain: If you do not set the ignorableWhitespace-callback for SAX, all spaces are reported as normal characters (see parser.c:2831). That’s what NSXMLParser seems to do. libxml isn’t good in regards to ignorable whitespaces, it currently uses a heuristic to detect them instead of checking the schema (see parser.c:2863).

One could argue if calling parser(_:foundIgnorableWhitespace:) for some ignorable whitespaces (based on an heuristic!) is a good idea. On the other hand, it being called is documented in the API contract.

What do you think about the following aproach: If the delegate implements parser(_:foundIgnorableWhitespace:), we call it (as libxml suggests), hoping for libxml getting better one day. If the delegate does not implement the callback, we handle it as is, forwarding the call to parser(_:foundCharacters:) for all whitespaces.

[dhoepfl]

Any change this gets merged?

Regarding the conflict: Am I supposed to keep the pull request up to date? How is this typically handled?
Resolving the conflict is easy: Accepting both changes will probably work (WASI should be before SR-13546).

That WASI-changeset seems rather unfinished:

• It always fails on init?(contentsOf url: URL).
• It completely removes init(stream: InputStream) (which is part of the public API) when WASI is defined.

[MaxDesiatov]

The WASI API is partial on purpose, this platform doesn't support streams, multithreading, and file access.

As for merging, I assume this needs an approval from @millenomi.

[parkera]

Closing older PRs after re-core of swift-corelibs-foundation on swift-foundation (#5001).

[dhoepfl]

Closing older PRs after re-core of swift-corelibs-foundation on swift-foundation (#5001).

Are the underlying bugs fixed in the new implementation? The code does not look any different. I even mentioned that the Darwin implementation is affected, too.

[parkera]

Can we rebase this on top of the new main so the conflicts are resolved?

[dhoepfl]

Can we rebase this on top of the new main so the conflicts are resolved?

I locally rebased, it was not a big thing. But unfortunately, since even main fails to compile on my machine, I cannot push it due to my “do not release untested code policy”:

swift-corelibs-foundation/Sources/CoreFoundation/internalInclude/CFInternal.h:102:10: fatal error: 'Block.h' file not found

It’s probably not a big thing but honestly, I am no longer willing to burn any more of my precious time. Apple has known about these bugs for several years, I even provided a fix for free, making sure it can be merged without conflict for quite a while. Fixing (these) bugs obviously has no priority at Apple.

Maybe it would be better to just deprecate or remove XMLParser. IMHO, at its current state, no-one should use it. It will fail for valid input.