[XMLParser] Fix reentrancy issue around currentParser (#5061)

* [XMLParser] Use `TaskLocal` for storing the current parser

Instead of thread-local storage, use `TaskLocal` to store the current
parser. This solves three issues:

1. If someone calls `XMLParser.parse()` with a new parser instance in
   a delegate method call, it overwrote the current parser and wrote
   it back after the call as `nil`, not the previous current parser.
   This reentrancy issue can be a problem especially when someone uses
   external entity resolving since the feature depends on the current
   parser tracking. Using `TaskLocal` solves this issue since it tracks
   values as a stack and restores the previous value at the end of the
   `withValue` call.
2. Since jobs of different tasks can be scheduled on the same thread,
   different tasks can refer to the same thread-local storage. This
   wouldn't be a problem for now since the `parse()` method doesn't
   have any suspention points and different tasks can't run on the same
   thread during the parsing. However, it's better to use `TaskLocal`
   to leverage the concurrency model of Swift.
3. The global variable `_currentParser` existed in the WASI platform
   path but it's unsafe in the Swift concurrency model. It wouldn't be a
   problem on WASI since it's always single-threaded, we should avoid
   platform-specific assumption as much as possible.

* Remove unnecessary `#if os(WASI)` condition in XMLParser.swift

* Keep the current parser in TLS instead of TaskLocal

TaskLocal storage is inherited by non-detached child tasks, which can
lead to the parser being shared between tasks. This is not our intention
and can lead to inconsistent state. Instead, we should keep the current
parser in thread-local storage. This should be safe as long as we don't
have any structured suspension points in `withCurrentParser` block.

* Simplify the current parser context management

Author: kateinoigakukun

Sources/FoundationXML/XMLParser.swift

@@ -398,9 +398,7 @@ extension XMLParser : @unchecked Sendable { }
 
 open class XMLParser : NSObject {
     private var _handler: _CFXMLInterfaceSAXHandler
-#if !os(WASI)
     internal var _stream: InputStream?
-#endif
     internal var _data: Data?
 
     internal var _chunkSize = Int(4096 * 32) // a suitably large number for a decent chunk size
@@ -414,9 +412,6 @@ open class XMLParser : NSObject {
 
     // initializes the parser with the specified URL.
     public convenience init?(contentsOf url: URL) {
-#if os(WASI)
-        return nil
-#else
         setupXMLParsing()
         if url.isFileURL {
             if let stream = InputStream(url: url) {
@@ -434,7 +429,6 @@ open class XMLParser : NSObject {
                 return nil
             }
         }
-#endif
     }
 
     // create the parser from data
@@ -450,15 +444,13 @@ open class XMLParser : NSObject {
         _CFXMLInterfaceDestroyContext(_parserContext)
     }
 
-#if !os(WASI)
     //create a parser that incrementally pulls data from the specified stream and parses it.
     public init(stream: InputStream) {
         setupXMLParsing()
         _stream = stream
         _handler = _CFXMLInterfaceCreateSAXHandler()
         _parserContext = nil
     }
-#endif
 
     open weak var delegate: XMLParserDelegate?
 
@@ -469,33 +461,35 @@ open class XMLParser : NSObject {
     open var externalEntityResolvingPolicy: ExternalEntityResolvingPolicy = .never
 
     open var allowedExternalEntityURLs: Set<URL>?
-    
-#if os(WASI)
-    private static var _currentParser: XMLParser?
-#endif
 
+    #if os(WASI)
+    /// The current parser associated with the current thread. (assuming no multi-threading)
+    /// FIXME: Unify the implementation with the other platforms once we unlock `threadDictionary`
+    ///        or migrate to `FoundationEssentials._ThreadLocal`.
+    private static nonisolated(unsafe) var _currentParser: XMLParser? = nil
+    #else
+    /// The current parser associated with the current thread.
+    private static var _currentParser: XMLParser? {
+        get {
+            return Thread.current.threadDictionary["__CurrentNSXMLParser"] as? XMLParser
+        }
+        set {
+            Thread.current.threadDictionary["__CurrentNSXMLParser"] = newValue
+        }
+    }
+    #endif
+
+    /// The current parser associated with the current thread.
     internal static func currentParser() -> XMLParser? {
-#if os(WASI)
         return _currentParser
-#else
-        if let current = Thread.current.threadDictionary["__CurrentNSXMLParser"] {
-            return current as? XMLParser
-        } else {
-            return nil
-        }
-#endif
     }
-    
-    internal static func setCurrentParser(_ parser: XMLParser?) {
-#if os(WASI)
+
+    /// Execute the given closure with the current parser set to the given parser.
+    internal static func withCurrentParser<R>(_ parser: XMLParser, _ body: () -> R) -> R {
+        let oldParser = _currentParser
         _currentParser = parser
-#else
-        if let p = parser {
-            Thread.current.threadDictionary["__CurrentNSXMLParser"] = p
-        } else {
-            Thread.current.threadDictionary.removeObject(forKey: "__CurrentNSXMLParser")
-        }
-#endif
+        defer { _currentParser = oldParser }
+        return body()
     }
 
     internal func _handleParseResult(_ parseResult: Int32) -> Bool {
@@ -569,7 +563,6 @@ open class XMLParser : NSObject {
         return result
     }
 
-#if !os(WASI)
     internal func parseFrom(_ stream : InputStream) -> Bool {
         var result = true
 
@@ -598,37 +591,17 @@ open class XMLParser : NSObject {
 
         return result
     }
-#else
-    internal func parse(from data: Data) -> Bool {
-        var result = true
-        var chunkStart = 0
-        var chunkEnd = min(_chunkSize, data.count)
-        while result && chunkStart < chunkEnd {
-            let chunk = data[chunkStart..<chunkEnd]
-            result = parseData(chunk)
-            chunkStart = chunkEnd
-            chunkEnd = min(chunkEnd + _chunkSize, data.count)
-        }
-        return result
-    }
-#endif
 
     // called to start the event-driven parse. Returns YES in the event of a successful parse, and NO in case of error.
     open func parse() -> Bool {
-#if os(WASI)
-        return _data.map { parse(from: $0) } ?? false
-#else
-        XMLParser.setCurrentParser(self)
-        defer { XMLParser.setCurrentParser(nil) }
-
-        if _stream != nil {
-            return parseFrom(_stream!)
-        } else if _data != nil {
-            return parseData(_data!, lastChunkOfData: true)
+        return Self.withCurrentParser(self) {
+            if _stream != nil {
+                return parseFrom(_stream!)
+            } else if _data != nil {
+                return parseData(_data!, lastChunkOfData: true)
+            }
+            return false
         }
-
-        return false
-#endif
     }
 
     // called by the delegate to stop the parse. The delegate will get an error message sent to it.

Tests/Foundation/TestXMLParser.swift

@@ -198,5 +198,46 @@ class TestXMLParser : XCTestCase {
         ElementNameChecker("noPrefix").check()
         ElementNameChecker("myPrefix:myLocalName").check()
     }
-    
+
+    func testExternalEntity() throws {
+        class Delegate: XMLParserDelegateEventStream {
+            override func parserDidStartDocument(_ parser: XMLParser) {
+                // Start a child parser, updating `currentParser` to the child parser
+                // to ensure that `currentParser` won't be reset to `nil`, which would
+                // ignore any external entity related configuration.
+                let childParser = XMLParser(data: "<child />".data(using: .utf8)!)
+                XCTAssertTrue(childParser.parse())
+                super.parserDidStartDocument(parser)
+            }
+        }
+        try withTemporaryDirectory { dir, _ in
+            let greetingPath = dir.appendingPathComponent("greeting.xml")
+            try Data("<hello />".utf8).write(to: greetingPath)
+            let xml = """
+            <?xml version="1.0" standalone="no"?>
+            <!DOCTYPE doc [
+              <!ENTITY greeting SYSTEM "\(greetingPath.absoluteString)">
+            ]>
+            <doc>&greeting;</doc>
+            """
+
+            let parser = XMLParser(data: xml.data(using: .utf8)!)
+            // Explicitly disable external entity resolving
+            parser.externalEntityResolvingPolicy = .never
+            let delegate = Delegate()
+            parser.delegate = delegate
+            // The parse result changes depending on the libxml2 version
+            // because of the following libxml2 commit (shipped in libxml2 2.9.10):
+            // https://gitlab.gnome.org/GNOME/libxml2/-/commit/eddfbc38fa7e84ccd480eab3738e40d1b2c83979
+            // So we don't check the parse result here.
+            _ = parser.parse()
+            XCTAssertEqual(delegate.events, [
+                .startDocument,
+                .didStartElement("doc", nil, nil, [:]),
+                // Should not have parsed the external entity
+                .didEndElement("doc", nil, nil),
+                .endDocument,
+            ])
+        }
+    }
 }