Improve circular reference detection in grok processor (elastic#74581)

danhermann · Jul 1, 2021 · c6d7d58 · c6d7d58
1 parent bc3be6e
commit c6d7d58
Show file tree

Hide file tree

Showing 2 changed files with 43 additions and 12 deletions.
diff --git a/libs/grok/src/main/java/org/elasticsearch/grok/Grok.java b/libs/grok/src/main/java/org/elasticsearch/grok/Grok.java
@@ -108,7 +108,21 @@ private Grok(Map<String, String> patternBank, String grokPattern, boolean namedC
      * check for a circular reference.
      */
     private void forbidCircularReferences(String patternName, List<String> path, String pattern) {
-        if (pattern.contains("%{" + patternName + "}") || pattern.contains("%{" + patternName + ":")) {
+        // first ensure that the pattern bank contains no simple circular references (i.e., any pattern
+        // containing an immediate reference to itself) as those can cause the remainder of this algorithm
+        // to recurse infinitely
+        for (Map.Entry<String, String> entry : patternBank.entrySet()) {
+            if (patternReferencesItself(entry.getValue(), entry.getKey())) {
+                throw new IllegalArgumentException("circular reference in pattern [" + entry.getKey() + "][" + entry.getValue() + "]");
+            }
+        }
+
+        // next recursively check any other pattern names referenced in the pattern
+        innerForbidCircularReferences(patternName, path, pattern);
+    }
+
+    private void innerForbidCircularReferences(String patternName, List<String> path, String pattern) {
+        if (patternReferencesItself(pattern, patternName)) {
             String message;
             if (path.isEmpty()) {
                 message = "circular reference in pattern [" + patternName + "][" + pattern + "]";
@@ -123,17 +137,18 @@ private void forbidCircularReferences(String patternName, List<String> path, Str
             throw new IllegalArgumentException(message);
         }
 
+        // next check any other pattern names found in the pattern
         for (int i = pattern.indexOf("%{"); i != -1; i = pattern.indexOf("%{", i + 1)) {
             int begin = i + 2;
-            int brackedIndex = pattern.indexOf('}', begin);
+            int bracketIndex = pattern.indexOf('}', begin);
             int columnIndex = pattern.indexOf(':', begin);
             int end;
-            if (brackedIndex != -1 && columnIndex == -1) {
-                end = brackedIndex;
-            } else if (columnIndex != -1 && brackedIndex == -1) {
+            if (bracketIndex != -1 && columnIndex == -1) {
+                end = bracketIndex;
+            } else if (columnIndex != -1 && bracketIndex == -1) {
                 end = columnIndex;
-            } else if (brackedIndex != -1 && columnIndex != -1) {
-                end = Math.min(brackedIndex, columnIndex);
+            } else if (bracketIndex != -1 && columnIndex != -1) {
+                end = Math.min(bracketIndex, columnIndex);
             } else {
                 throw new IllegalArgumentException("pattern [" + pattern + "] has circular references to other pattern definitions");
             }
@@ -143,6 +158,10 @@ private void forbidCircularReferences(String patternName, List<String> path, Str
         }
     }
 
+    private static boolean patternReferencesItself(String pattern, String patternName) {
+        return pattern.contains("%{" + patternName + "}") || pattern.contains("%{" + patternName + ":");
+    }
+
     private String groupMatch(String name, Region region, String pattern) {
         try {
             int number = GROK_PATTERN_REGEX.nameToBackrefNumber(name.getBytes(StandardCharsets.UTF_8), 0,

diff --git a/libs/grok/src/test/java/org/elasticsearch/grok/GrokTests.java b/libs/grok/src/test/java/org/elasticsearch/grok/GrokTests.java
@@ -320,8 +320,7 @@ public void testCircularReference() {
             String pattern = "%{NAME1}";
             new Grok(bank, pattern, false, logger::warn);
         });
-        assertEquals("circular reference in pattern [NAME3][!!!%{NAME1}!!!] back to pattern [NAME1] via patterns [NAME2]",
-            e.getMessage());
+        assertEquals("circular reference in pattern [NAME3][!!!%{NAME1}!!!] back to pattern [NAME1] via patterns [NAME2]", e.getMessage());
 
         e = expectThrows(IllegalArgumentException.class, () -> {
             Map<String, String> bank = new TreeMap<>();
@@ -331,10 +330,23 @@ public void testCircularReference() {
             bank.put("NAME4", "!!!%{NAME5}!!!");
             bank.put("NAME5", "!!!%{NAME1}!!!");
             String pattern = "%{NAME1}";
-            new Grok(bank, pattern, false, logger::warn );
+            new Grok(bank, pattern, false, logger::warn);
+        });
+        assertEquals(
+            "circular reference in pattern [NAME5][!!!%{NAME1}!!!] back to pattern [NAME1] via patterns [NAME2=>NAME3=>NAME4]",
+            e.getMessage()
+        );
+    }
+
+    public void testCircularSelfReference() {
+        Exception e = expectThrows(IllegalArgumentException.class, () -> {
+            Map<String, String> bank = new HashMap<>();
+            bank.put("ANOTHER", "%{INT}");
+            bank.put("INT", "%{INT}");
+            String pattern = "does_not_matter";
+            new Grok(bank, pattern, false, logger::warn);
         });
-        assertEquals("circular reference in pattern [NAME5][!!!%{NAME1}!!!] back to pattern [NAME1] " +
-            "via patterns [NAME2=>NAME3=>NAME4]", e.getMessage());
+        assertEquals("circular reference in pattern [INT][%{INT}]", e.getMessage());
     }
 
     public void testBooleanCaptures() {