From 3ebe2629ff83b5a8bcf7438f06b83425e75064eb Mon Sep 17 00:00:00 2001 From: "copilot-swe-agent[bot]" <198982749+Copilot@users.noreply.github.com> Date: Thu, 14 May 2026 12:13:11 +0000 Subject: [PATCH] Fix NPE in XMLHandlerImpl.dispose caused by concurrent DOM serialization Agent-Logs-Url: https://github.com/OpenIdentityPlatform/OpenICF/sessions/dd83ece1-f572-4dea-8b5e-958430aa6c51 Co-authored-by: vharseko <6818498+vharseko@users.noreply.github.com> --- .../connectors/xml/ConcurrentXMLHandler.java | 15 ++- .../connectors/xml/XMLHandlerImpl.java | 105 +++++++++++------- 2 files changed, 77 insertions(+), 43 deletions(-) diff --git a/OpenICF-xml-connector/src/main/java/org/forgerock/openicf/connectors/xml/ConcurrentXMLHandler.java b/OpenICF-xml-connector/src/main/java/org/forgerock/openicf/connectors/xml/ConcurrentXMLHandler.java index a20a10b2..9a3716a5 100644 --- a/OpenICF-xml-connector/src/main/java/org/forgerock/openicf/connectors/xml/ConcurrentXMLHandler.java +++ b/OpenICF-xml-connector/src/main/java/org/forgerock/openicf/connectors/xml/ConcurrentXMLHandler.java @@ -111,7 +111,10 @@ public Uid authenticate(String username, GuardedString password) { } public XMLHandler init() { - lock.readLock().lock(); + // Use the write lock so the invokers counter and proxy.init() + // are not racing against a concurrent dispose() (or another init()) + // running under a shared read lock. + lock.writeLock().lock(); try { if (0 == invokers) { proxy.init(); @@ -119,13 +122,17 @@ public XMLHandler init() { invokers++; } finally { - lock.readLock().unlock(); + lock.writeLock().unlock(); } return this; } public void dispose() { - lock.readLock().lock(); + // Use the write lock so that proxy.dispose() (which serializes the + // shared DOM through Saxon) cannot run concurrently with itself or + // with any read-lock operation (search/authenticate) that walks the + // same DOM. Mutating the invokers counter also requires exclusion. + lock.writeLock().lock(); try { invokers--; if (0 == invokers) { @@ -133,7 +140,7 @@ public void dispose() { } } finally { - lock.readLock().unlock(); + lock.writeLock().unlock(); } } diff --git a/OpenICF-xml-connector/src/main/java/org/forgerock/openicf/connectors/xml/XMLHandlerImpl.java b/OpenICF-xml-connector/src/main/java/org/forgerock/openicf/connectors/xml/XMLHandlerImpl.java index f854a5c2..d5521263 100644 --- a/OpenICF-xml-connector/src/main/java/org/forgerock/openicf/connectors/xml/XMLHandlerImpl.java +++ b/OpenICF-xml-connector/src/main/java/org/forgerock/openicf/connectors/xml/XMLHandlerImpl.java @@ -388,47 +388,74 @@ public void dispose() { } try { - try { - XPathFactory xpathFactory = new net.sf.saxon.xpath.XPathFactoryImpl(); - // XPath to find empty text nodes. - XPathExpression xpathExp = xpathFactory.newXPath().compile("//text()[normalize-space(.) = '']"); - NodeList emptyTextNodes = null; - emptyTextNodes = (NodeList) xpathExp.evaluate(document, XPathConstants.NODESET); - - // Remove each empty text node from document. - for (int i = 0; i < emptyTextNodes.getLength(); i++) { - Node emptyTextNode = emptyTextNodes.item(i); - emptyTextNode.getParentNode().removeChild(emptyTextNode); + // Synchronize on the document so that XPath cleanup and the + // Saxon transform happen atomically with respect to any other + // thread that might still hold a reference to the same DOM. + // Saxon's DOMSender walks children via NodeList.item(i) after + // calling getLength(); a concurrent removeChild on the DOM + // can make item(i) return null and trigger a NullPointerException + // in DOMSender.walkNode. + synchronized (document) { + try { + XPathFactory xpathFactory = new net.sf.saxon.xpath.XPathFactoryImpl(); + // XPath to find empty text nodes. + XPathExpression xpathExp = xpathFactory.newXPath().compile("//text()[normalize-space(.) = '']"); + NodeList emptyTextNodes = (NodeList) xpathExp.evaluate(document, XPathConstants.NODESET); + + // Snapshot the list before mutating the DOM, then remove + // each empty text node (guarding against nodes whose + // parent has already been detached). + int len = emptyTextNodes.getLength(); + List toRemove = new ArrayList(len); + for (int i = 0; i < len; i++) { + Node n = emptyTextNodes.item(i); + if (n != null) { + toRemove.add(n); + } + } + for (Node emptyTextNode : toRemove) { + Node parent = emptyTextNode.getParentNode(); + if (parent != null) { + parent.removeChild(emptyTextNode); + } + } + } catch (XPathExpressionException e) { + //We don't care. It's just formatting. } - } catch (XPathExpressionException e) { - //We don't care. It's just formatting. - } - TransformerFactory transformerFactory = new net.sf.saxon.TransformerFactoryImpl(); - Transformer transformer = transformerFactory.newTransformer(); - transformer.setOutputProperty(OutputKeys.INDENT, "yes"); - transformer.setOutputProperty(OutputKeys.METHOD, "xml"); - transformer.setOutputProperty(OutputKeys.ENCODING, "UTF-8"); - transformer.setOutputProperty("{http://xml.apache.org/xslt}indent-amount", "2"); - - DOMSource source = new DOMSource(document); - /* Running this code in java 5 we had to change - StreamResult result = new StreamResult(config.getXmlFilePath()); - into - StreamResult result = new StreamResult(config.getXmlFilePath().getPath()); - Otherwise you get the following error: - javax.xml.transform.TransformerException: java.io.FileNotFoundException: - */ - /* - * If the safePath is not escaped then it throws - * net.sf.saxon.trans.XPathException: java.net.URISyntaxException: - * Illegal character in safePath at index 9: /temp/XML Connector/test.xml - * String safePath = config.getXmlFilePath().getPath().replaceAll(" ", "%20"); - */ - FileOutputStream fos = new FileOutputStream(config.getXmlFilePath()); - StreamResult result = new StreamResult(fos); - - transformer.transform(source, result); + TransformerFactory transformerFactory = new net.sf.saxon.TransformerFactoryImpl(); + Transformer transformer = transformerFactory.newTransformer(); + transformer.setOutputProperty(OutputKeys.INDENT, "yes"); + transformer.setOutputProperty(OutputKeys.METHOD, "xml"); + transformer.setOutputProperty(OutputKeys.ENCODING, "UTF-8"); + transformer.setOutputProperty("{http://xml.apache.org/xslt}indent-amount", "2"); + + DOMSource source = new DOMSource(document); + /* Running this code in java 5 we had to change + StreamResult result = new StreamResult(config.getXmlFilePath()); + into + StreamResult result = new StreamResult(config.getXmlFilePath().getPath()); + Otherwise you get the following error: + javax.xml.transform.TransformerException: java.io.FileNotFoundException: + */ + /* + * If the safePath is not escaped then it throws + * net.sf.saxon.trans.XPathException: java.net.URISyntaxException: + * Illegal character in safePath at index 9: /temp/XML Connector/test.xml + * String safePath = config.getXmlFilePath().getPath().replaceAll(" ", "%20"); + */ + FileOutputStream fos = new FileOutputStream(config.getXmlFilePath()); + try { + StreamResult result = new StreamResult(fos); + transformer.transform(source, result); + } finally { + try { + fos.close(); + } catch (IOException ioe) { + log.warn("Failed to close XML output stream: {0}", ioe); + } + } + } log.info("Saving changes to xml file"); } catch (TransformerException ex) {