8162431: CatalogUriResolver with circular/self referencing catalog file is not throwing CatalogException as expected

Reviewed-by: lancea

jaxp/src/java.xml/share/classes/javax/xml/catalog/CatalogImpl.java

@@ -31,6 +31,7 @@ import java.net.URI;
 import java.net.URISyntaxException;
 import java.net.URL;
 import java.util.ArrayList;
+import java.util.HashMap;
 import java.util.Iterator;
 import java.util.List;
 import java.util.NoSuchElementException;
@@ -72,9 +73,6 @@ class CatalogImpl extends GroupEntry implements Catalog {
     //System Id for this catalog
     String systemId;
 
-    //The parent
-    CatalogImpl parent = null;
-
     /*
      A list of catalog entry files from the input, excluding the current catalog.
      Paths in the List are normalized.
@@ -107,7 +105,7 @@ class CatalogImpl extends GroupEntry implements Catalog {
      * catalog file.
      */
     public CatalogImpl(CatalogImpl parent, CatalogFeatures f, String... file) throws CatalogException {
-        super(CatalogEntryType.CATALOG);
+        super(CatalogEntryType.CATALOG, parent);
         if (f == null) {
             throw new NullPointerException(
                     formatMessage(CatalogMessages.ERR_NULL_ARGUMENT, new Object[]{"CatalogFeatures"}));
@@ -117,7 +115,7 @@ class CatalogImpl extends GroupEntry implements Catalog {
             CatalogMessages.reportNPEOnNull("The path to the catalog file", file[0]);
         }
 
-        init(parent, f);
+        init(f);
 
         //Path of catalog files
         String[] catalogFile = file;
@@ -159,19 +157,34 @@ class CatalogImpl extends GroupEntry implements Catalog {
                     }
                 }
             }
-
-            if (systemId != null) {
-                parse(systemId);
-            }
         }
     }
 
-    private void init(CatalogImpl parent, CatalogFeatures f) {
+    /**
-        this.parent = parent;
+     * Loads the catalog
+     */
+    void load() {
+        if (systemId != null) {
+            parse(systemId);
+        }
+
+        //save this catalog before loading the next
+        loadedCatalogs.put(systemId, this);
+
+        //Load delegate and alternative catalogs if defer is false.
+        if (!isDeferred()) {
+           loadDelegateCatalogs();
+           loadNextCatalogs();
+        }
+    }
+
+    private void init(CatalogFeatures f) {
         if (parent == null) {
             level = 0;
         } else {
             level = parent.level + 1;
+            this.loadedCatalogs = parent.loadedCatalogs;
+            this.catalogsSearched = parent.catalogsSearched;
         }
         if (f == null) {
             this.features = CatalogFeatures.defaults();
@@ -196,11 +209,6 @@ class CatalogImpl extends GroupEntry implements Catalog {
         entries.stream().filter((entry) -> (entry.type == CatalogEntryType.GROUP)).forEach((entry) -> {
             ((GroupEntry) entry).reset();
         });
-
-        if (parent != null) {
-            this.loadedCatalogs = parent.loadedCatalogs;
-            this.catalogsSearched = parent.catalogsSearched;
-        }
     }
 
     /**
@@ -421,16 +429,16 @@ class CatalogImpl extends GroupEntry implements Catalog {
     void loadNextCatalogs() {
         //loads catalogs specified in nextCatalogs
         if (nextCatalogs != null) {
-            for (NextCatalog next : nextCatalogs) {
+            nextCatalogs.stream().forEach((next) -> {
                 getCatalog(next.getCatalogURI());
-            }
+            });
         }
 
         //loads catalogs from the input list
         if (inputFiles != null) {
-            for (String file : inputFiles) {
+            inputFiles.stream().forEach((file) -> {
                 getCatalog(getSystemId(file));
-            }
+            });
         }
     }
 
@@ -445,14 +453,14 @@ class CatalogImpl extends GroupEntry implements Catalog {
             return null;
         }
 
-        Catalog c = null;
+        CatalogImpl c = null;
         String path = uri.toASCIIString();
 
         if (verifyCatalogFile(uri)) {
             c = getLoadedCatalog(path);
             if (c == null) {
                 c = new CatalogImpl(this, features, path);
-                saveLoadedCatalog(path, c);
+                c.load();
             }
         }
         return c;
@@ -464,7 +472,7 @@ class CatalogImpl extends GroupEntry implements Catalog {
      * @param catalogId the catalogId associated with the Catalog object
      * @param c the Catalog to be saved
      */
-    void saveLoadedCatalog(String catalogId, Catalog c) {
+    void saveLoadedCatalog(String catalogId, CatalogImpl c) {
         loadedCatalogs.put(catalogId, c);
     }
 

jaxp/src/java.xml/share/classes/javax/xml/catalog/CatalogManager.java

@@ -62,7 +62,9 @@ public final class CatalogManager {
      * @throws CatalogException If an error occurs while parsing the catalog
      */
     public static Catalog catalog(CatalogFeatures features, String... paths) {
-        return new CatalogImpl(features, paths);
+        CatalogImpl catalog = new CatalogImpl(features, paths);
+        catalog.load();
+        return catalog;
     }
 
     /**

jaxp/src/java.xml/share/classes/javax/xml/catalog/CatalogReader.java

@@ -259,15 +259,6 @@ class CatalogReader extends DefaultHandler implements EntityResolver, URIResolve
         CatalogEntryType type = CatalogEntryType.getType(localName);
         if (type == CatalogEntryType.GROUP) {
             inGroup = false;
-        } else if (type == CatalogEntryType.CATALOGENTRY) {
-            /*
-             Done reading the catalog file.
-             Load delegate and alternative catalogs if defer is false.
-             */
-            if (!catalog.isDeferred()) {
-                catalog.loadDelegateCatalogs();
-                catalog.loadNextCatalogs();
-            }
         }
     }
 

jaxp/src/java.xml/share/classes/javax/xml/catalog/CatalogResolverImpl.java

@@ -119,6 +119,10 @@ final class CatalogResolverImpl implements CatalogResolver {
         String result = null;
         CatalogImpl c = (CatalogImpl)catalog;
         String uri = Normalizer.normalizeURI(href);
+        if (uri == null) {
+            return null;
+        }
+
         //check whether uri is an urn
         if (uri != null && uri.startsWith(Util.URN)) {
             String publicId = Normalizer.decodeURN(uri);

jaxp/src/java.xml/share/classes/javax/xml/catalog/GroupEntry.java

@@ -48,6 +48,9 @@ class GroupEntry extends BaseEntry {
     //Value of the prefer attribute
     boolean isPreferPublic = true;
 
+    //The parent of the catalog instance
+    CatalogImpl parent = null;
+
     //The catalog instance this group belongs to
     CatalogImpl catalog;
 
@@ -55,10 +58,10 @@ class GroupEntry extends BaseEntry {
     List<BaseEntry> entries = new ArrayList<>();
 
     //loaded delegated catalog by system id
-    Map<String, Catalog> delegateCatalogs = new HashMap<>();
+    Map<String, CatalogImpl> delegateCatalogs = new HashMap<>();
 
     //A list of all loaded Catalogs, including this, and next catalogs
-    Map<String, Catalog> loadedCatalogs = new HashMap<>();
+    Map<String, CatalogImpl> loadedCatalogs = new HashMap<>();
 
     /*
      A list of Catalog Ids that have already been searched in a matching
@@ -136,8 +139,9 @@ class GroupEntry extends BaseEntry {
      *
      * @param type The type of the entry
      */
-    public GroupEntry(CatalogEntryType type) {
+    public GroupEntry(CatalogEntryType type, CatalogImpl parent) {
         super(type);
+        this.parent = parent;
     }
 
     /**
@@ -163,7 +167,7 @@ class GroupEntry extends BaseEntry {
     }
     /**
      * Constructs a group entry.
-     * @param catalog The parent catalog
+     * @param catalog The catalog this GroupEntry belongs
      * @param base The baseURI attribute
      * @param attributes The attributes
      */
@@ -445,13 +449,14 @@ class GroupEntry extends BaseEntry {
      * @param catalogId the catalog Id
      */
     Catalog loadCatalog(URI catalogURI) {
-        Catalog delegateCatalog = null;
+        CatalogImpl delegateCatalog = null;
         if (catalogURI != null) {
             String catalogId = catalogURI.toASCIIString();
             delegateCatalog = getLoadedCatalog(catalogId);
             if (delegateCatalog == null) {
                 if (verifyCatalogFile(catalogURI)) {
                     delegateCatalog = new CatalogImpl(catalog, features, catalogId);
+                    delegateCatalog.load();
                     delegateCatalogs.put(catalogId, delegateCatalog);
                 }
             }
@@ -467,8 +472,8 @@ class GroupEntry extends BaseEntry {
      * @return a Catalog object previously loaded, or null if none in the saved
      * list
      */
-    Catalog getLoadedCatalog(String catalogId) {
+    CatalogImpl getLoadedCatalog(String catalogId) {
-        Catalog c = null;
+        CatalogImpl c = null;
 
         //checl delegate Catalogs
         c = delegateCatalogs.get(catalogId);
@@ -504,7 +509,7 @@ class GroupEntry extends BaseEntry {
         }
 
         String catalogId = catalogURI.toASCIIString();
-        if (catalogsSearched.contains(catalogId)) {
+        if (catalogsSearched.contains(catalogId) || isCircular(catalogId)) {
             CatalogMessages.reportRunTimeError(CatalogMessages.ERR_CIRCULAR_REFERENCE,
                     new Object[]{CatalogMessages.sanitize(catalogId)});
         }
@@ -512,4 +517,20 @@ class GroupEntry extends BaseEntry {
         return true;
     }
 
+    /**
+     * Checks whether the catalog is circularly referenced
+     * @param systemId the system identifier of the catalog to be loaded
+     * @return true if is circular, false otherwise
+     */
+    boolean isCircular(String systemId) {
+        if (parent == null) {
+            return false;
+        }
+
+        if (parent.systemId.equals(systemId)) {
+            return true;
+        }
+
+        return parent.isCircular(systemId);
+    }
 }

jaxp/test/javax/xml/jaxp/functional/catalog/DeferFeatureTest.java

@@ -64,12 +64,12 @@ public class DeferFeatureTest {
     public Object[][] data() {
         return new Object[][]{
             // By default, alternative catalogs are not loaded.
-            {createCatalog(CatalogFeatures.defaults()), 0},
+            {createCatalog(CatalogFeatures.defaults()), 1},
             // Alternative catalogs are not loaded when DEFER is set to true.
-            {createCatalog(createDeferFeature(DEFER_TRUE)), 0},
+            {createCatalog(createDeferFeature(DEFER_TRUE)), 1},
-            // The 3 alternative catalogs are not pre-loaded
+            // The 3 alternative catalogs are pre-loaded along with the parent
             //when DEFER is set to false.
-            {createCatalog(createDeferFeature(DEFER_FALSE)), 3}};
+            {createCatalog(createDeferFeature(DEFER_FALSE)), 4}};
     }
 
     private CatalogFeatures createDeferFeature(String defer) {

jaxp/test/javax/xml/jaxp/unittest/catalog/CatalogTest.java

@@ -92,6 +92,34 @@ public class CatalogTest extends CatalogSupportBase {
         super.setUp();
     }
 
+    /*
+     * @bug 8162431
+     * Verifies that circular references are caught and
+     * CatalogException is thrown.
+     */
+    @Test(dataProvider = "getFeatures", expectedExceptions = CatalogException.class)
+    public void testCircularRef(CatalogFeatures cf, String xml) {
+        CatalogResolver catalogResolver = CatalogManager.catalogResolver(
+                cf,
+                getClass().getResource(xml).getFile());
+        catalogResolver.resolve("anyuri", "");
+    }
+
+    /*
+       DataProvider: used to verify circular reference
+        Data columns: CatalogFeatures, catalog
+     */
+    @DataProvider(name = "getFeatures")
+    public Object[][] getFeatures() {
+
+        return new Object[][]{
+            {CatalogFeatures.builder().with(CatalogFeatures.Feature.DEFER, "false").build(),
+                "catalogReferCircle-itself.xml"},
+            {CatalogFeatures.defaults(), "catalogReferCircle-itself.xml"},
+            {CatalogFeatures.builder().with(CatalogFeatures.Feature.DEFER, "false").build(),
+                "catalogReferCircle-left.xml"},
+            {CatalogFeatures.defaults(), "catalogReferCircle-left.xml"},};
+    }
 
     /*
      * @bug 8163232

jaxp/test/javax/xml/jaxp/unittest/catalog/catalogReferCircle-itself.xml

@@ -0,0 +1,6 @@
+<?xml version="1.0" encoding="UTF-8"?>
+
+<catalog xmlns="urn:oasis:names:tc:entity:xmlns:xml:catalog">
+    <nextCatalog catalog="catalogReferCircle-itself.xml" />
+</catalog>
+

jaxp/test/javax/xml/jaxp/unittest/catalog/catalogReferCircle-left.xml

@@ -0,0 +1,6 @@
+<?xml version="1.0" encoding="UTF-8"?>
+
+<catalog xmlns="urn:oasis:names:tc:entity:xmlns:xml:catalog">
+    <nextCatalog catalog="catalogReferCircle-right.xml" />
+</catalog>
+

jaxp/test/javax/xml/jaxp/unittest/catalog/catalogReferCircle-right.xml

@@ -0,0 +1,6 @@
+<?xml version="1.0" encoding="UTF-8"?>
+
+<catalog xmlns="urn:oasis:names:tc:entity:xmlns:xml:catalog">
+    <nextCatalog catalog="catalogReferCircle-left.xml" />
+</catalog>
+