From 377b2a4f5da524b590ed31506db4b8db8ea02e27 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?J=C3=B6rg=20Kubitz?= Date: Fri, 23 Jun 2023 07:38:37 +0200 Subject: [PATCH] Factory for SAXParser parsing XML without DOCTYPE External entity resolution is not supported by PDE (see PDECoreMessages.XMLErrorReporter_ExternalEntityResolution) but still the SAXParser did follow external links where DefaultHandler.resolveEntity was not overwritten. At many places PDE already overwrote DefaultHandler.resolveEntity to prevent external resolution. With the new configuration that method is not even called anymore. This change offers and uses a configuration that * reports an Exception if .xml contains DOCTYPE or * does just ignore external references (as a fall back if the exception would show up to cause trouble). Also the caching of used parsers in possibly other threads is removed because the SAXParser is not guaranteed to be thread-safe. Only the factory is reused, because that is effectively final after creation. Reusing SAXParser is not a big help nowadays - see XmlParserFactoryTest.main(String[]) for performance test. In my measurement successive parser creations takes only ~ 0.06 ms. --- .../api/tools/internal/APIFileGenerator.java | 11 +- .../tools/internal/model/BundleComponent.java | 11 +- .../internal/search/UseReportConverter.java | 5 +- .../tools/internal/search/UseScanParser.java | 6 +- .../APIDeprecationReportConversionTask.java | 5 +- .../tasks/APIFreezeReportConversionTask.java | 5 +- .../tasks/AnalysisReportConversionTask.java | 5 +- .../internal/build/tasks/JNLPGenerator.java | 5 +- .../build/tasks/XmlParserFactory.java | 42 ++++ .../pde/internal/core/AbstractModel.java | 4 +- .../core/plugin/AbstractExtensions.java | 4 +- .../pde/internal/core/plugin/PluginBase.java | 6 +- .../configurator/ConfigurationParser.java | 6 +- .../update/configurator/FeatureParser.java | 7 +- .../pde/internal/core/util/PDEXMLHelper.java | 31 --- .../internal/core/util/SAXParserWrapper.java | 22 +- .../internal/core/util/XmlParserFactory.java | 76 +++++++ .../core/tests/internal/AllPDECoreTests.java | 1 + .../tests/internal/XmlParserFactoryTest.java | 203 ++++++++++++++++++ 19 files changed, 354 insertions(+), 101 deletions(-) create mode 100644 build/org.eclipse.pde.build/src_ant/org/eclipse/pde/internal/build/tasks/XmlParserFactory.java create mode 100644 ui/org.eclipse.pde.core/src/org/eclipse/pde/internal/core/util/XmlParserFactory.java create mode 100644 ui/org.eclipse.pde.ui.tests/src/org/eclipse/pde/core/tests/internal/XmlParserFactoryTest.java diff --git a/apitools/org.eclipse.pde.api.tools/src/org/eclipse/pde/api/tools/internal/APIFileGenerator.java b/apitools/org.eclipse.pde.api.tools/src/org/eclipse/pde/api/tools/internal/APIFileGenerator.java index 47b15e6e18..f215781646 100644 --- a/apitools/org.eclipse.pde.api.tools/src/org/eclipse/pde/api/tools/internal/APIFileGenerator.java +++ b/apitools/org.eclipse.pde.api.tools/src/org/eclipse/pde/api/tools/internal/APIFileGenerator.java @@ -32,10 +32,8 @@ import java.util.zip.ZipEntry; import java.util.zip.ZipFile; -import javax.xml.parsers.FactoryConfigurationError; import javax.xml.parsers.ParserConfigurationException; import javax.xml.parsers.SAXParser; -import javax.xml.parsers.SAXParserFactory; import org.eclipse.core.runtime.CoreException; import org.eclipse.jdt.core.JavaCore; @@ -48,6 +46,7 @@ import org.eclipse.pde.api.tools.internal.provisional.model.IApiTypeContainer; import org.eclipse.pde.api.tools.internal.provisional.scanner.TagScanner; import org.eclipse.pde.api.tools.internal.util.Util; +import org.eclipse.pde.internal.core.util.XmlParserFactory; import org.osgi.framework.BundleException; import org.osgi.framework.Constants; import org.xml.sax.Attributes; @@ -459,15 +458,9 @@ private boolean isAPIToolsNature(File dotProjectFile) { * @return true if it contains a source extension point, false otherwise */ private boolean containsAPIToolsNature(String pluginXMLContents) { - SAXParserFactory factory = null; - try { - factory = SAXParserFactory.newInstance(); - } catch (FactoryConfigurationError e) { - return false; - } SAXParser saxParser = null; try { - saxParser = factory.newSAXParser(); + saxParser = XmlParserFactory.createSAXParserIgnoringDOCTYPE(); } catch (ParserConfigurationException | SAXException e) { // ignore } diff --git a/apitools/org.eclipse.pde.api.tools/src/org/eclipse/pde/api/tools/internal/model/BundleComponent.java b/apitools/org.eclipse.pde.api.tools/src/org/eclipse/pde/api/tools/internal/model/BundleComponent.java index b33b3ad998..feca9f1f64 100644 --- a/apitools/org.eclipse.pde.api.tools/src/org/eclipse/pde/api/tools/internal/model/BundleComponent.java +++ b/apitools/org.eclipse.pde.api.tools/src/org/eclipse/pde/api/tools/internal/model/BundleComponent.java @@ -36,10 +36,8 @@ import java.util.zip.ZipEntry; import java.util.zip.ZipFile; -import javax.xml.parsers.FactoryConfigurationError; import javax.xml.parsers.ParserConfigurationException; import javax.xml.parsers.SAXParser; -import javax.xml.parsers.SAXParserFactory; import org.eclipse.core.runtime.CoreException; import org.eclipse.core.runtime.IPath; @@ -79,6 +77,7 @@ import org.eclipse.pde.api.tools.internal.util.Util; import org.eclipse.pde.internal.core.TargetWeaver; import org.eclipse.pde.internal.core.util.ManifestUtils; +import org.eclipse.pde.internal.core.util.XmlParserFactory; import org.osgi.framework.BundleException; import org.osgi.framework.Constants; import org.osgi.framework.Version; @@ -1068,15 +1067,9 @@ private static boolean isSourceComponent(Map manifest, File loca * @return true if it contains a source extension point, false otherwise */ private static boolean containsSourceExtensionPoint(String pluginXMLContents) { - SAXParserFactory factory = null; - try { - factory = SAXParserFactory.newInstance(); - } catch (FactoryConfigurationError e) { - return false; - } SAXParser saxParser = null; try { - saxParser = factory.newSAXParser(); + saxParser = XmlParserFactory.createSAXParserWithErrorOnDOCTYPE(); } catch (ParserConfigurationException | SAXException e) { // ignore } diff --git a/apitools/org.eclipse.pde.api.tools/src/org/eclipse/pde/api/tools/internal/search/UseReportConverter.java b/apitools/org.eclipse.pde.api.tools/src/org/eclipse/pde/api/tools/internal/search/UseReportConverter.java index 0cae1b47c0..29eab3af08 100644 --- a/apitools/org.eclipse.pde.api.tools/src/org/eclipse/pde/api/tools/internal/search/UseReportConverter.java +++ b/apitools/org.eclipse.pde.api.tools/src/org/eclipse/pde/api/tools/internal/search/UseReportConverter.java @@ -38,7 +38,6 @@ import javax.xml.parsers.ParserConfigurationException; import javax.xml.parsers.SAXParser; -import javax.xml.parsers.SAXParserFactory; import javax.xml.transform.Result; import javax.xml.transform.Source; import javax.xml.transform.Transformer; @@ -66,6 +65,7 @@ import org.eclipse.pde.api.tools.internal.provisional.search.IMetadata; import org.eclipse.pde.api.tools.internal.util.Signatures; import org.eclipse.pde.api.tools.internal.util.Util; +import org.eclipse.pde.internal.core.util.XmlParserFactory; import org.eclipse.pde.internal.core.util.XmlTransformerFactory; import org.osgi.framework.Version; import org.w3c.dom.Element; @@ -741,9 +741,8 @@ protected List parse(IProgressMonitor monitor) throws Exception { */ SAXParser getParser() throws Exception { if (this.parser == null) { - SAXParserFactory factory = SAXParserFactory.newInstance(); try { - this.parser = factory.newSAXParser(); + this.parser = XmlParserFactory.createSAXParserWithErrorOnDOCTYPE(); } catch (ParserConfigurationException pce) { throw new Exception(SearchMessages.UseReportConverter_pce_error_getting_parser, pce); } catch (SAXException se) { diff --git a/apitools/org.eclipse.pde.api.tools/src/org/eclipse/pde/api/tools/internal/search/UseScanParser.java b/apitools/org.eclipse.pde.api.tools/src/org/eclipse/pde/api/tools/internal/search/UseScanParser.java index 0fc1ac9316..ec8e79e8e3 100644 --- a/apitools/org.eclipse.pde.api.tools/src/org/eclipse/pde/api/tools/internal/search/UseScanParser.java +++ b/apitools/org.eclipse.pde.api.tools/src/org/eclipse/pde/api/tools/internal/search/UseScanParser.java @@ -24,7 +24,6 @@ import javax.xml.parsers.ParserConfigurationException; import javax.xml.parsers.SAXParser; -import javax.xml.parsers.SAXParserFactory; import org.eclipse.core.runtime.IProgressMonitor; import org.eclipse.core.runtime.SubMonitor; @@ -36,6 +35,7 @@ import org.eclipse.pde.api.tools.internal.provisional.descriptors.IComponentDescriptor; import org.eclipse.pde.api.tools.internal.provisional.descriptors.IMemberDescriptor; import org.eclipse.pde.api.tools.internal.util.Util; +import org.eclipse.pde.internal.core.util.XmlParserFactory; import org.xml.sax.Attributes; import org.xml.sax.SAXException; import org.xml.sax.helpers.DefaultHandler; @@ -307,9 +307,9 @@ public void parse(String xmlLocation, IProgressMonitor monitor, UseScanVisitor u * builds */ SAXParser getParser() throws Exception { - SAXParserFactory factory = SAXParserFactory.newInstance(); try { - return factory.newSAXParser(); + return XmlParserFactory.createSAXParserWithErrorOnDOCTYPE(); + } catch (ParserConfigurationException pce) { throw new Exception(SearchMessages.UseReportConverter_pce_error_getting_parser, pce); } catch (SAXException se) { diff --git a/apitools/org.eclipse.pde.api.tools/src_ant/org/eclipse/pde/api/tools/internal/tasks/APIDeprecationReportConversionTask.java b/apitools/org.eclipse.pde.api.tools/src_ant/org/eclipse/pde/api/tools/internal/tasks/APIDeprecationReportConversionTask.java index d5ae67f2b0..b222016dda 100644 --- a/apitools/org.eclipse.pde.api.tools/src_ant/org/eclipse/pde/api/tools/internal/tasks/APIDeprecationReportConversionTask.java +++ b/apitools/org.eclipse.pde.api.tools/src_ant/org/eclipse/pde/api/tools/internal/tasks/APIDeprecationReportConversionTask.java @@ -25,7 +25,6 @@ import javax.xml.parsers.ParserConfigurationException; import javax.xml.parsers.SAXParser; -import javax.xml.parsers.SAXParserFactory; import org.apache.tools.ant.BuildException; import org.apache.tools.ant.Task; @@ -34,6 +33,7 @@ import org.eclipse.pde.api.tools.internal.IApiXmlConstants; import org.eclipse.pde.api.tools.internal.provisional.comparator.IDelta; import org.eclipse.pde.api.tools.internal.util.Util; +import org.eclipse.pde.internal.core.util.XmlParserFactory; import org.xml.sax.Attributes; import org.xml.sax.SAXException; import org.xml.sax.helpers.DefaultHandler; @@ -311,10 +311,9 @@ public void execute() throws BuildException { System.out.println("xmlFileLocation : " + this.xmlFileLocation); //$NON-NLS-1$ System.out.println("htmlFileLocation : " + this.htmlFileLocation); //$NON-NLS-1$ } - SAXParserFactory factory = SAXParserFactory.newInstance(); SAXParser parser = null; try { - parser = factory.newSAXParser(); + parser = XmlParserFactory.createSAXParserWithErrorOnDOCTYPE(); } catch (ParserConfigurationException | SAXException e) { e.printStackTrace(); } diff --git a/apitools/org.eclipse.pde.api.tools/src_ant/org/eclipse/pde/api/tools/internal/tasks/APIFreezeReportConversionTask.java b/apitools/org.eclipse.pde.api.tools/src_ant/org/eclipse/pde/api/tools/internal/tasks/APIFreezeReportConversionTask.java index ecd076ee70..6e0411113e 100644 --- a/apitools/org.eclipse.pde.api.tools/src_ant/org/eclipse/pde/api/tools/internal/tasks/APIFreezeReportConversionTask.java +++ b/apitools/org.eclipse.pde.api.tools/src_ant/org/eclipse/pde/api/tools/internal/tasks/APIFreezeReportConversionTask.java @@ -25,7 +25,6 @@ import javax.xml.parsers.ParserConfigurationException; import javax.xml.parsers.SAXParser; -import javax.xml.parsers.SAXParserFactory; import org.apache.tools.ant.BuildException; import org.apache.tools.ant.Task; @@ -34,6 +33,7 @@ import org.eclipse.pde.api.tools.internal.IApiXmlConstants; import org.eclipse.pde.api.tools.internal.provisional.comparator.IDelta; import org.eclipse.pde.api.tools.internal.util.Util; +import org.eclipse.pde.internal.core.util.XmlParserFactory; import org.xml.sax.Attributes; import org.xml.sax.SAXException; import org.xml.sax.helpers.DefaultHandler; @@ -491,10 +491,9 @@ public void execute() throws BuildException { } } } - SAXParserFactory factory = SAXParserFactory.newInstance(); SAXParser parser = null; try { - parser = factory.newSAXParser(); + parser = XmlParserFactory.createSAXParserWithErrorOnDOCTYPE(); } catch (ParserConfigurationException | SAXException e) { e.printStackTrace(); } diff --git a/apitools/org.eclipse.pde.api.tools/src_ant/org/eclipse/pde/api/tools/internal/tasks/AnalysisReportConversionTask.java b/apitools/org.eclipse.pde.api.tools/src_ant/org/eclipse/pde/api/tools/internal/tasks/AnalysisReportConversionTask.java index 2a4b416503..985e7865e0 100644 --- a/apitools/org.eclipse.pde.api.tools/src_ant/org/eclipse/pde/api/tools/internal/tasks/AnalysisReportConversionTask.java +++ b/apitools/org.eclipse.pde.api.tools/src_ant/org/eclipse/pde/api/tools/internal/tasks/AnalysisReportConversionTask.java @@ -27,7 +27,6 @@ import javax.xml.parsers.ParserConfigurationException; import javax.xml.parsers.SAXParser; -import javax.xml.parsers.SAXParserFactory; import org.apache.tools.ant.BuildException; import org.apache.tools.ant.Task; @@ -35,6 +34,7 @@ import org.eclipse.pde.api.tools.internal.IApiXmlConstants; import org.eclipse.pde.api.tools.internal.provisional.ApiPlugin; import org.eclipse.pde.api.tools.internal.util.Util; +import org.eclipse.pde.internal.core.util.XmlParserFactory; import org.xml.sax.Attributes; import org.xml.sax.SAXException; import org.xml.sax.helpers.DefaultHandler; @@ -429,10 +429,9 @@ public void execute() throws BuildException { if (!this.reportsRoot.exists() || !this.reportsRoot.isDirectory()) { throw new BuildException(NLS.bind(Messages.invalid_directory_name, this.xmlReportsLocation)); } - SAXParserFactory factory = SAXParserFactory.newInstance(); SAXParser parser = null; try { - parser = factory.newSAXParser(); + parser = XmlParserFactory.createSAXParserWithErrorOnDOCTYPE(); } catch (ParserConfigurationException | SAXException e) { e.printStackTrace(); } diff --git a/build/org.eclipse.pde.build/src_ant/org/eclipse/pde/internal/build/tasks/JNLPGenerator.java b/build/org.eclipse.pde.build/src_ant/org/eclipse/pde/internal/build/tasks/JNLPGenerator.java index af3abf2909..3fe1fb2a8b 100644 --- a/build/org.eclipse.pde.build/src_ant/org/eclipse/pde/internal/build/tasks/JNLPGenerator.java +++ b/build/org.eclipse.pde.build/src_ant/org/eclipse/pde/internal/build/tasks/JNLPGenerator.java @@ -37,7 +37,6 @@ import javax.xml.parsers.ParserConfigurationException; import javax.xml.parsers.SAXParser; -import javax.xml.parsers.SAXParserFactory; import org.xml.sax.Attributes; import org.xml.sax.InputSource; @@ -66,7 +65,6 @@ public class JNLPGenerator extends DefaultHandler { * feature.includes = extension * feature.plugin = jar */ - private final static SAXParserFactory parserFactory = SAXParserFactory.newInstance(); private PrintWriter out; private String destination; private String provider; @@ -109,8 +107,7 @@ public JNLPGenerator(String feature, String destination, String codebase, String this.locale = locale; this.generateOfflineAllowed = generateOfflineAllowed; try { - parserFactory.setNamespaceAware(true); - parser = parserFactory.newSAXParser(); + parser = XmlParserFactory.createNsAwareSAXParserWithErrorOnDOCTYPE(); } catch (ParserConfigurationException | SAXException e) { System.out.println(e); } diff --git a/build/org.eclipse.pde.build/src_ant/org/eclipse/pde/internal/build/tasks/XmlParserFactory.java b/build/org.eclipse.pde.build/src_ant/org/eclipse/pde/internal/build/tasks/XmlParserFactory.java new file mode 100644 index 0000000000..db56c5607e --- /dev/null +++ b/build/org.eclipse.pde.build/src_ant/org/eclipse/pde/internal/build/tasks/XmlParserFactory.java @@ -0,0 +1,42 @@ +/******************************************************************************* + * Copyright (c) 2023 Joerg Kubitz and others. + * + * This program and the accompanying materials + * are made available under the terms of the Eclipse Public License 2.0 + * which accompanies this distribution, and is available at + * https://www.eclipse.org/legal/epl-2.0/ + * + * SPDX-License-Identifier: EPL-2.0 + *******************************************************************************/ +package org.eclipse.pde.internal.build.tasks; + +import javax.xml.parsers.ParserConfigurationException; +import javax.xml.parsers.SAXParser; +import javax.xml.parsers.SAXParserFactory; + +import org.xml.sax.SAXException; + +public class XmlParserFactory { + private XmlParserFactory() { + // static Utility only + } + + private static final SAXParserFactory SAX_FACTORY_ERROR_ON_DOCTYPE_NS = createSAXFactoryWithErrorOnDOCTYPE(); + + private static synchronized SAXParserFactory createSAXFactoryWithErrorOnDOCTYPE() { + SAXParserFactory f = SAXParserFactory.newInstance(); + f.setNamespaceAware(true); + try { + // force org.xml.sax.SAXParseException for any DOCTYPE: + f.setFeature("http://apache.org/xml/features/disallow-doctype-decl", true); //$NON-NLS-1$ + } catch (Exception e) { + throw new RuntimeException(e); + } + return f; + } + + public static SAXParser createNsAwareSAXParserWithErrorOnDOCTYPE() throws ParserConfigurationException, SAXException { + return SAX_FACTORY_ERROR_ON_DOCTYPE_NS.newSAXParser(); + } + +} diff --git a/ui/org.eclipse.pde.core/src/org/eclipse/pde/internal/core/AbstractModel.java b/ui/org.eclipse.pde.core/src/org/eclipse/pde/internal/core/AbstractModel.java index 7e8f58690a..817b5ce10b 100644 --- a/ui/org.eclipse.pde.core/src/org/eclipse/pde/internal/core/AbstractModel.java +++ b/ui/org.eclipse.pde.core/src/org/eclipse/pde/internal/core/AbstractModel.java @@ -24,7 +24,6 @@ import javax.xml.parsers.FactoryConfigurationError; import javax.xml.parsers.ParserConfigurationException; import javax.xml.parsers.SAXParser; -import javax.xml.parsers.SAXParserFactory; import org.eclipse.core.filebuffers.FileBuffers; import org.eclipse.core.filebuffers.ITextFileBuffer; @@ -41,6 +40,7 @@ import org.eclipse.pde.core.IModelChangedEvent; import org.eclipse.pde.core.IModelChangedListener; import org.eclipse.pde.core.ModelChangedEvent; +import org.eclipse.pde.internal.core.util.XmlParserFactory; import org.xml.sax.SAXException; public abstract class AbstractModel extends PlatformObject implements IModel, IModelChangeProviderExtension, Serializable { @@ -237,7 +237,7 @@ public void throwParseErrorsException(Throwable e) throws CoreException { } protected SAXParser getSaxParser() throws ParserConfigurationException, SAXException, FactoryConfigurationError { - return SAXParserFactory.newInstance().newSAXParser(); + return XmlParserFactory.createSAXParserWithErrorOnDOCTYPE(); } @Override diff --git a/ui/org.eclipse.pde.core/src/org/eclipse/pde/internal/core/plugin/AbstractExtensions.java b/ui/org.eclipse.pde.core/src/org/eclipse/pde/internal/core/plugin/AbstractExtensions.java index 6f08358a6b..e044ae2b1b 100644 --- a/ui/org.eclipse.pde.core/src/org/eclipse/pde/internal/core/plugin/AbstractExtensions.java +++ b/ui/org.eclipse.pde.core/src/org/eclipse/pde/internal/core/plugin/AbstractExtensions.java @@ -23,7 +23,6 @@ import java.util.List; import javax.xml.parsers.ParserConfigurationException; -import javax.xml.parsers.SAXParserFactory; import org.eclipse.core.resources.IFile; import org.eclipse.core.runtime.CoreException; @@ -36,6 +35,7 @@ import org.eclipse.pde.core.plugin.ISharedPluginModel; import org.eclipse.pde.internal.core.PDECore; import org.eclipse.pde.internal.core.PDECoreMessages; +import org.eclipse.pde.internal.core.util.XmlParserFactory; import org.w3c.dom.Node; import org.xml.sax.SAXException; @@ -199,7 +199,7 @@ public String getSchemaVersion() { try { InputStream stream = new BufferedInputStream(((IFile) res).getContents(true)); PluginHandler handler = new PluginHandler(true); - SAXParserFactory.newInstance().newSAXParser().parse(stream, handler); + XmlParserFactory.createSAXParserWithErrorOnDOCTYPE().parse(stream, handler); return handler.getSchemaVersion(); } catch (CoreException | SAXException | IOException | ParserConfigurationException e) { } diff --git a/ui/org.eclipse.pde.core/src/org/eclipse/pde/internal/core/plugin/PluginBase.java b/ui/org.eclipse.pde.core/src/org/eclipse/pde/internal/core/plugin/PluginBase.java index de70b7354c..055099ebfd 100644 --- a/ui/org.eclipse.pde.core/src/org/eclipse/pde/internal/core/plugin/PluginBase.java +++ b/ui/org.eclipse.pde.core/src/org/eclipse/pde/internal/core/plugin/PluginBase.java @@ -20,7 +20,6 @@ import javax.xml.parsers.FactoryConfigurationError; import javax.xml.parsers.ParserConfigurationException; import javax.xml.parsers.SAXParser; -import javax.xml.parsers.SAXParserFactory; import org.eclipse.core.runtime.CoreException; import org.eclipse.osgi.service.resolver.BundleDescription; @@ -36,6 +35,7 @@ import org.eclipse.pde.internal.core.PDECoreMessages; import org.eclipse.pde.internal.core.PDEState; import org.eclipse.pde.internal.core.bundle.BundlePluginBase; +import org.eclipse.pde.internal.core.util.XmlParserFactory; import org.osgi.framework.Version; import org.w3c.dom.Node; import org.w3c.dom.NodeList; @@ -404,7 +404,8 @@ protected boolean hasRequiredAttributes() { } protected SAXParser getSaxParser() throws ParserConfigurationException, SAXException, FactoryConfigurationError { - return SAXParserFactory.newInstance().newSAXParser(); + return XmlParserFactory.createSAXParserWithErrorOnDOCTYPE(); + } public static int getMatchRule(VersionRange versionRange) { @@ -447,6 +448,7 @@ public String getBundleSourceEntry() { return fBundleSourceEntry; } + @Override public boolean exportsExternalAnnotations() { return fExportsExternalAnnotations; } diff --git a/ui/org.eclipse.pde.core/src/org/eclipse/pde/internal/core/update/configurator/ConfigurationParser.java b/ui/org.eclipse.pde.core/src/org/eclipse/pde/internal/core/update/configurator/ConfigurationParser.java index ede90d4784..986aa6cca9 100644 --- a/ui/org.eclipse.pde.core/src/org/eclipse/pde/internal/core/update/configurator/ConfigurationParser.java +++ b/ui/org.eclipse.pde.core/src/org/eclipse/pde/internal/core/update/configurator/ConfigurationParser.java @@ -26,10 +26,10 @@ import javax.xml.parsers.ParserConfigurationException; import javax.xml.parsers.SAXParser; -import javax.xml.parsers.SAXParserFactory; import org.eclipse.core.runtime.URIUtil; import org.eclipse.osgi.util.NLS; +import org.eclipse.pde.internal.core.util.XmlParserFactory; import org.xml.sax.Attributes; import org.xml.sax.InputSource; import org.xml.sax.SAXException; @@ -43,7 +43,6 @@ class ConfigurationParser extends DefaultHandler implements IConfigurationConsta private static final String URL_PROPERTY = "org.eclipse.update.resolution_url"; //$NON-NLS-1$ private static final String EMPTY_STRING = ""; //$NON-NLS-1$ - private final static SAXParserFactory parserFactory = SAXParserFactory.newInstance(); private SAXParser parser; private URL currentSiteURL; @@ -58,8 +57,7 @@ class ConfigurationParser extends DefaultHandler implements IConfigurationConsta public ConfigurationParser() throws InvocationTargetException { try { - parserFactory.setNamespaceAware(true); - this.parser = parserFactory.newSAXParser(); + this.parser = XmlParserFactory.createSAXParserWithErrorOnDOCTYPE(true); } catch (ParserConfigurationException | SAXException e) { Utils.log(Utils.newStatus("ConfigurationParser", e)); //$NON-NLS-1$ throw new InvocationTargetException(e); diff --git a/ui/org.eclipse.pde.core/src/org/eclipse/pde/internal/core/update/configurator/FeatureParser.java b/ui/org.eclipse.pde.core/src/org/eclipse/pde/internal/core/update/configurator/FeatureParser.java index b373d0b1cb..5f82d1d100 100644 --- a/ui/org.eclipse.pde.core/src/org/eclipse/pde/internal/core/update/configurator/FeatureParser.java +++ b/ui/org.eclipse.pde.core/src/org/eclipse/pde/internal/core/update/configurator/FeatureParser.java @@ -20,9 +20,9 @@ import javax.xml.parsers.ParserConfigurationException; import javax.xml.parsers.SAXParser; -import javax.xml.parsers.SAXParserFactory; import org.eclipse.osgi.util.NLS; +import org.eclipse.pde.internal.core.util.XmlParserFactory; import org.xml.sax.Attributes; import org.xml.sax.InputSource; import org.xml.sax.SAXException; @@ -40,16 +40,13 @@ class FeatureParser extends DefaultHandler { private FeatureEntry feature; private URL url; - private final static SAXParserFactory parserFactory = SAXParserFactory.newInstance(); - /** * Constructs a feature parser. */ public FeatureParser() { super(); try { - parserFactory.setNamespaceAware(true); - this.parser = parserFactory.newSAXParser(); + this.parser = XmlParserFactory.createSAXParserWithErrorOnDOCTYPE(true); } catch (ParserConfigurationException | SAXException e) { System.out.println(e); } diff --git a/ui/org.eclipse.pde.core/src/org/eclipse/pde/internal/core/util/PDEXMLHelper.java b/ui/org.eclipse.pde.core/src/org/eclipse/pde/internal/core/util/PDEXMLHelper.java index a54b8045e4..ea50bc7c83 100644 --- a/ui/org.eclipse.pde.core/src/org/eclipse/pde/internal/core/util/PDEXMLHelper.java +++ b/ui/org.eclipse.pde.core/src/org/eclipse/pde/internal/core/util/PDEXMLHelper.java @@ -22,10 +22,6 @@ import javax.xml.parsers.DocumentBuilderFactory; import javax.xml.parsers.FactoryConfigurationError; import javax.xml.parsers.ParserConfigurationException; -import javax.xml.parsers.SAXParser; -import javax.xml.parsers.SAXParserFactory; - -import org.xml.sax.SAXException; /** * PDEXMLHelper @@ -33,40 +29,20 @@ */ public class PDEXMLHelper { - protected static SAXParserFactory fSAXFactory; protected static PDEXMLHelper fPinstance; protected static DocumentBuilderFactory fDOMFactory; - protected static List> fSAXParserQueue; protected static List> fDOMParserQueue; protected static int fSAXPoolLimit; protected static int fDOMPoolLimit; protected static final int FMAXPOOLLIMIT = 1; protected PDEXMLHelper() throws FactoryConfigurationError { - fSAXFactory = SAXParserFactory.newInstance(); fDOMFactory = XmlDocumentBuilderFactory.createDocumentBuilderFactoryWithErrorOnDOCTYPE(); - fSAXParserQueue = Collections.synchronizedList(new LinkedList<>()); fDOMParserQueue = Collections.synchronizedList(new LinkedList<>()); fSAXPoolLimit = FMAXPOOLLIMIT; fDOMPoolLimit = FMAXPOOLLIMIT; } - public synchronized SAXParser getDefaultSAXParser() throws ParserConfigurationException, SAXException { - - SAXParser parser = null; - if (fSAXParserQueue.isEmpty()) { - parser = fSAXFactory.newSAXParser(); - } else { - SoftReference reference = fSAXParserQueue.remove(0); - if (reference.get() != null) { - parser = reference.get(); - } else { - parser = fSAXFactory.newSAXParser(); - } - } - return parser; - } - public synchronized DocumentBuilder getDefaultDOMParser() throws ParserConfigurationException { DocumentBuilder parser = null; @@ -90,13 +66,6 @@ public static PDEXMLHelper Instance() throws FactoryConfigurationError { return fPinstance; } - public synchronized void recycleSAXParser(SAXParser parser) { - if (fSAXParserQueue.size() < fSAXPoolLimit) { - SoftReference reference = new SoftReference<>(parser); - fSAXParserQueue.add(reference); - } - } - public synchronized void recycleDOMParser(DocumentBuilder parser) { if (fDOMParserQueue.size() < fDOMPoolLimit) { SoftReference reference = new SoftReference<>(parser); diff --git a/ui/org.eclipse.pde.core/src/org/eclipse/pde/internal/core/util/SAXParserWrapper.java b/ui/org.eclipse.pde.core/src/org/eclipse/pde/internal/core/util/SAXParserWrapper.java index 1ab43db0a7..352c981829 100644 --- a/ui/org.eclipse.pde.core/src/org/eclipse/pde/internal/core/util/SAXParserWrapper.java +++ b/ui/org.eclipse.pde.core/src/org/eclipse/pde/internal/core/util/SAXParserWrapper.java @@ -36,28 +36,14 @@ private SAXParserWrapper() { // static use only public static void parse(File f, DefaultHandler dh) throws SAXException, IOException, ParserConfigurationException, FactoryConfigurationError { - parse(p -> p.parse(f, dh)); + SAXParser fParser = XmlParserFactory.createSAXParserWithErrorOnDOCTYPE(); + fParser.parse(f, dh); } public static void parse(InputStream is, DefaultHandler dh) throws SAXException, IOException, ParserConfigurationException, FactoryConfigurationError { - parse(p -> p.parse(is, dh)); + SAXParser fParser = XmlParserFactory.createSAXParserWithErrorOnDOCTYPE(); + fParser.parse(is, dh); } - // NOTE: If other parser method calls are required, the corresponding - // wrapper method needs to be added here - - private interface ParseProcess { - void parseWith(SAXParser parser) throws SAXException, IOException; - } - - private static void parse(ParseProcess pp) - throws ParserConfigurationException, SAXException, FactoryConfigurationError, IOException { - SAXParser fParser = PDEXMLHelper.Instance().getDefaultSAXParser(); - try { - pp.parseWith(fParser); - } finally { - PDEXMLHelper.Instance().recycleSAXParser(fParser); - } - } } diff --git a/ui/org.eclipse.pde.core/src/org/eclipse/pde/internal/core/util/XmlParserFactory.java b/ui/org.eclipse.pde.core/src/org/eclipse/pde/internal/core/util/XmlParserFactory.java new file mode 100644 index 0000000000..979865fa6c --- /dev/null +++ b/ui/org.eclipse.pde.core/src/org/eclipse/pde/internal/core/util/XmlParserFactory.java @@ -0,0 +1,76 @@ +/******************************************************************************* + * Copyright (c) 2023 Joerg Kubitz and others. + * + * This program and the accompanying materials + * are made available under the terms of the Eclipse Public License 2.0 + * which accompanies this distribution, and is available at + * https://www.eclipse.org/legal/epl-2.0/ + * + * SPDX-License-Identifier: EPL-2.0 + *******************************************************************************/ +package org.eclipse.pde.internal.core.util; + +import javax.xml.XMLConstants; +import javax.xml.parsers.ParserConfigurationException; +import javax.xml.parsers.SAXParser; +import javax.xml.parsers.SAXParserFactory; + +import org.xml.sax.SAXException; +import org.xml.sax.SAXNotRecognizedException; +import org.xml.sax.SAXNotSupportedException; + +public class XmlParserFactory { + private XmlParserFactory() { + // static Utility only + } + + private static final SAXParserFactory SAX_FACTORY_ERROR_ON_DOCTYPE = createSAXFactoryWithErrorOnDOCTYPE(false); + private static final SAXParserFactory SAX_FACTORY_ERROR_ON_DOCTYPE_NS = createSAXFactoryWithErrorOnDOCTYPE(true); + private static final SAXParserFactory SAX_FACTORY_IGNORING_DOCTYPE = createSAXFactoryIgnoringDOCTYPE(); + + private static synchronized SAXParserFactory createSAXFactoryWithErrorOnDOCTYPE(boolean awareness) { + SAXParserFactory f = SAXParserFactory.newInstance(); + f.setNamespaceAware(awareness); + try { + // force org.xml.sax.SAXParseException for any DOCTYPE: + f.setFeature("http://apache.org/xml/features/disallow-doctype-decl", true); //$NON-NLS-1$ + } catch (Exception e) { + throw new RuntimeException(e); + } + return f; + } + + private static synchronized SAXParserFactory createSAXFactoryIgnoringDOCTYPE() { + SAXParserFactory f = SAXParserFactory.newInstance(); + try { + // ignore DOCTYPE: + f.setFeature("http://xml.org/sax/features/external-general-entities", false); //$NON-NLS-1$ + f.setFeature("http://xml.org/sax/features/external-parameter-entities", false); //$NON-NLS-1$ + f.setFeature("http://apache.org/xml/features/nonvalidating/load-external-dtd", false); //$NON-NLS-1$ + } catch (Exception e) { + throw new RuntimeException(e); + } + return f; + } + + public static SAXParser createSAXParserWithErrorOnDOCTYPE() throws ParserConfigurationException, SAXException { + return createSAXParserWithErrorOnDOCTYPE(false); + } + + public static SAXParser createSAXParserWithErrorOnDOCTYPE(boolean awareness) + throws ParserConfigurationException, SAXException { + if (awareness) { + return SAX_FACTORY_ERROR_ON_DOCTYPE_NS.newSAXParser(); + } + return SAX_FACTORY_ERROR_ON_DOCTYPE.newSAXParser(); + } + + public static SAXParser createSAXParserIgnoringDOCTYPE() + throws ParserConfigurationException, SAXNotRecognizedException, SAXNotSupportedException, SAXException { + SAXParser parser = SAX_FACTORY_IGNORING_DOCTYPE.newSAXParser(); + parser.setProperty(XMLConstants.ACCESS_EXTERNAL_DTD, ""); //$NON-NLS-1$ + parser.setProperty(XMLConstants.ACCESS_EXTERNAL_SCHEMA, ""); //$NON-NLS-1$ + return parser; + } + +} diff --git a/ui/org.eclipse.pde.ui.tests/src/org/eclipse/pde/core/tests/internal/AllPDECoreTests.java b/ui/org.eclipse.pde.ui.tests/src/org/eclipse/pde/core/tests/internal/AllPDECoreTests.java index 1c8deea918..67aaa7cc66 100644 --- a/ui/org.eclipse.pde.ui.tests/src/org/eclipse/pde/core/tests/internal/AllPDECoreTests.java +++ b/ui/org.eclipse.pde.ui.tests/src/org/eclipse/pde/core/tests/internal/AllPDECoreTests.java @@ -11,6 +11,7 @@ XmlTransformerTest.class, // WorkspaceModelManagerTest.class, // WorkspaceProductModelManagerTest.class, // + XmlParserFactoryTest.class // }) public class AllPDECoreTests { } diff --git a/ui/org.eclipse.pde.ui.tests/src/org/eclipse/pde/core/tests/internal/XmlParserFactoryTest.java b/ui/org.eclipse.pde.ui.tests/src/org/eclipse/pde/core/tests/internal/XmlParserFactoryTest.java new file mode 100644 index 0000000000..5257f808ca --- /dev/null +++ b/ui/org.eclipse.pde.ui.tests/src/org/eclipse/pde/core/tests/internal/XmlParserFactoryTest.java @@ -0,0 +1,203 @@ +/******************************************************************************* + * Copyright (c) 2023 Joerg Kubitz and others. + * + * This program and the accompanying materials + * are made available under the terms of the Eclipse Public License 2.0 + * which accompanies this distribution, and is available at + * https://www.eclipse.org/legal/epl-2.0/ + * + * SPDX-License-Identifier: EPL-2.0 + *******************************************************************************/ +package org.eclipse.pde.core.tests.internal; + +import static org.junit.Assert.assertEquals; +import static org.junit.Assert.assertFalse; +import static org.junit.Assert.assertTrue; + +import java.io.BufferedReader; +import java.io.ByteArrayInputStream; +import java.io.IOException; +import java.io.InputStream; +import java.io.InputStreamReader; +import java.io.OutputStream; +import java.lang.reflect.InvocationTargetException; +import java.net.ServerSocket; +import java.net.Socket; +import java.net.SocketException; +import java.net.URI; +import java.nio.charset.StandardCharsets; +import java.nio.file.Files; +import java.nio.file.Path; +import java.util.ArrayList; +import java.util.Collection; +import java.util.List; +import java.util.concurrent.ConcurrentLinkedQueue; +import java.util.function.IntFunction; + +import javax.xml.parsers.SAXParser; + +import org.eclipse.pde.internal.core.util.XmlParserFactory; +import org.junit.Test; +import org.xml.sax.InputSource; +import org.xml.sax.SAXException; +import org.xml.sax.SAXParseException; +import org.xml.sax.helpers.DefaultHandler; + +public class XmlParserFactoryTest { + + List tmpFiles = new ArrayList<>(); + + @Test + public void testParseXmlWithExternalEntity() throws Exception { + SAXParser parser =XmlParserFactory.createSAXParserWithErrorOnDOCTYPE(); + try { + testParseXmlWithExternalEntity(parser, this::createMalciousXml); + assertTrue("SAXParseException expected", false); + } catch (SAXParseException e) { + String message = e.getMessage(); + assertTrue(message, message.contains("DOCTYPE")); + assertTrue(message, message.contains("http://apache.org/xml/features/disallow-doctype-decl")); + } + } + + @Test + public void testParseXmlWithoutExternalEntity() throws Exception { + SAXParser parser = XmlParserFactory.createSAXParserWithErrorOnDOCTYPE(); + testParseXmlWithExternalEntity(parser, this::createNormalXml); + } + + @Test + public void testParseXmlWithIgnoredExternalEntity() throws Exception { + SAXParser parser = XmlParserFactory.createSAXParserIgnoringDOCTYPE(); + testParseXmlWithExternalEntity(parser, this::createMalciousXml); + } + + @Test + public void testParseXmlWithoutIgnoredExternalEntity() throws Exception { + SAXParser parser = XmlParserFactory.createSAXParserIgnoringDOCTYPE(); + testParseXmlWithExternalEntity(parser, this::createNormalXml); + } + + InputStream createMalciousXml(int localPort) { + try { + Path tempSecret = Files.createTempFile("test", ".txt"); + tmpFiles.add(tempSecret); + Files.writeString(tempSecret, "secret"); + Path tempDtd = Files.createTempFile("test", ".dtd"); + tmpFiles.add(tempDtd); + String dtdContent = "\n" // + + "\n" // + + "\">\n" // + + "%var2;\n"; + Files.writeString(tempDtd, dtdContent); + StringBuilder sb = new StringBuilder(); + sb.append("\n"); + URI dtdURi = tempDtd.toUri(); + String dtdUrl = dtdURi.toURL().toString(); + sb.append("\n"); + sb.append("&var3;&var4;"); + String xml = sb.toString(); + return new ByteArrayInputStream(xml.getBytes(StandardCharsets.UTF_8)); + } catch (IOException e) { + throw new RuntimeException(e); + } + } + + InputStream createNormalXml(int localPort) { + StringBuilder sb = new StringBuilder(); + sb.append("\n"); + sb.append("hello"); + String xml = sb.toString(); + return new ByteArrayInputStream(xml.getBytes(StandardCharsets.UTF_8)); + } + + private void cleanup() throws IOException { + for (Path path : tmpFiles) { + Files.delete(path); + } + } + + public void testParseXmlWithExternalEntity(SAXParser parser, IntFunction xmlSupplier) + throws Exception { + Collection exceptionsInOtherThreads = new ConcurrentLinkedQueue<>(); + Thread httpServerThread = null; + try (ServerSocket serverSocket = new ServerSocket(0)) { + int localPort = serverSocket.getLocalPort(); + httpServerThread = new Thread("httpServerThread") { + @Override + public void run() { + try (Socket socket = serverSocket.accept()) { + try (BufferedReader in = new BufferedReader(new InputStreamReader(socket.getInputStream()))) { + String firstLine = in.readLine(); + try (OutputStream outputStream = socket.getOutputStream()) { + outputStream.write("HTTP/1.1 200 OK\r\n".getBytes(StandardCharsets.UTF_8)); + } + assertTrue(firstLine, firstLine.startsWith("GET")); + assertFalse("Server received secret: " + firstLine, firstLine.contains("secret")); // var3 + assertFalse("Server was contacted", true); + } + } catch (SocketException closed) { + // expected + } catch (Throwable e) { + exceptionsInOtherThreads.add(e); + } + } + }; + httpServerThread.start(); + List elements = new ArrayList<>(); + DefaultHandler handler = new DefaultHandler() { + @Override + public void startElement(String uri, String localName, String qName, org.xml.sax.Attributes attributes) + throws org.xml.sax.SAXException { + elements.add(qName); + } + + @Override + public void characters(char ch[], int start, int length) throws SAXException { + String content = new String(ch, start, length); + assertFalse("Secret was injected into xml: " + content, content.contains("secret")); // var4 + } + + @Override + public InputSource resolveEntity(String publicId, String systemId) throws IOException, SAXException { + // implementation that would do any remote call: + try { + return new InputSource(URI.create(systemId).toURL().openStream()); + } catch (IOException exception) { + throw new SAXException(exception); + } + // Also the default impl injects files: + // return null; + + // Does also prevent access to external files: + // return new InputSource(new StringReader("")); + } + + }; + try (InputStream xmlStream = xmlSupplier.apply(localPort)) { + parser.parse(xmlStream, handler); + } + assertEquals(List.of("Body"), elements); + } finally { + cleanup(); + } + httpServerThread.join(5000); + assertFalse(httpServerThread.isAlive()); + for (Throwable e : exceptionsInOtherThreads) { + throw new InvocationTargetException(e, e.getMessage()); + } + } + + static volatile Object sink; + + public static void main(String[] args) throws Exception { + // testParserCreatePerformance + for (int i = 1; i < 1000; i++) { + long n0 = System.nanoTime(); + sink = XmlParserFactory.createSAXParserIgnoringDOCTYPE(); + long n1 = System.nanoTime(); + System.out.println("run " + i + ": " + (n1 - n0) + "ns"); + // ~ run 999: 60000ns + } + } +}