From 2197776be67384d628f12a5def8663db9e4220cf Mon Sep 17 00:00:00 2001 From: David Smiley Date: Tue, 22 Sep 2020 15:46:59 -0400 Subject: [PATCH] SOLR-13181: param macro expansion could throw (#1877) ...StringIndexOutOfBoundsException on bad syntax * failOnMissingParams: should have been returning null (failing) on bad syntax cases Co-authored-by: Christine Poerschke --- solr/CHANGES.txt | 4 ++ .../solr/request/macro/MacroExpander.java | 35 +++++++----- .../solr/request/macro/TestMacroExpander.java | 54 +++++++++++++------ 3 files changed, 62 insertions(+), 31 deletions(-) diff --git a/solr/CHANGES.txt b/solr/CHANGES.txt index f4909e30537..109347b4c99 100644 --- a/solr/CHANGES.txt +++ b/solr/CHANGES.txt @@ -244,6 +244,10 @@ Bug Fixes * SOLR-14821: {!terms} docValuesTermsFilterTopLevel method now works with single-valued strings (Anatolii Siuniaev via Jason Gerlowski) +* SOLR-13181: macro expansion of parameters could result in a StringIndexOutOfBoundsException. + Also, params with macros will be processed more strictly by LTR FeatureWeight. + (David Smiley, Cesar Rodriguez, Christine Poerschke) + Other Changes --------------------- diff --git a/solr/core/src/java/org/apache/solr/request/macro/MacroExpander.java b/solr/core/src/java/org/apache/solr/request/macro/MacroExpander.java index 67219e53c93..144ce06b22c 100644 --- a/solr/core/src/java/org/apache/solr/request/macro/MacroExpander.java +++ b/solr/core/src/java/org/apache/solr/request/macro/MacroExpander.java @@ -119,8 +119,8 @@ public class MacroExpander { int start = 0; // start of the unprocessed part of the string StringBuilder sb = null; for (;;) { + assert idx >= start; idx = val.indexOf(macroStart, idx); - int matchedStart = idx; // check if escaped if (idx > 0) { @@ -135,18 +135,19 @@ public class MacroExpander { } } else if (idx < 0) { - if (sb == null) return val; - sb.append(val.substring(start)); - return sb.toString(); + break; } // found unescaped "${" - idx += macroStart.length(); + final int matchedStart = idx; - int rbrace = val.indexOf('}', idx); + int rbrace = val.indexOf('}', matchedStart + macroStart.length()); if (rbrace == -1) { // no matching close brace... - continue; + if (failOnMissingParams) { + return null; + } + break; } if (sb == null) { @@ -154,14 +155,14 @@ public class MacroExpander { } if (matchedStart > 0) { - sb.append(val.substring(start, matchedStart)); + sb.append(val, start, matchedStart); } // update "start" to be at the end of ${...} - start = rbrace + 1; + idx = start = rbrace + 1; - // String inbetween = val.substring(idx, rbrace); - StrParser parser = new StrParser(val, idx, rbrace); + // String in-between braces + StrParser parser = new StrParser(val, matchedStart + macroStart.length(), rbrace); try { String paramName = parser.getId(); String defVal = null; @@ -187,13 +188,19 @@ public class MacroExpander { } } catch (SyntaxError syntaxError) { + if (failOnMissingParams) { + return null; + } // append the part we would have skipped - sb.append( val.substring(matchedStart, start) ); - continue; + sb.append(val, matchedStart, start); } + } // loop idx + if (sb == null) { + return val; } - + sb.append(val, start, val.length()); + return sb.toString(); } diff --git a/solr/core/src/test/org/apache/solr/request/macro/TestMacroExpander.java b/solr/core/src/test/org/apache/solr/request/macro/TestMacroExpander.java index c6c60cb4481..99717e25318 100644 --- a/solr/core/src/test/org/apache/solr/request/macro/TestMacroExpander.java +++ b/solr/core/src/test/org/apache/solr/request/macro/TestMacroExpander.java @@ -16,14 +16,14 @@ */ package org.apache.solr.request.macro; -import java.util.Map; +import java.util.Collections; import java.util.HashMap; - +import java.util.Map; import org.apache.solr.SolrTestCase; import org.junit.Test; -/* +/** * Tests for the MacroExpander */ public class TestMacroExpander extends SolrTestCase { @@ -121,14 +121,13 @@ public class TestMacroExpander extends SolrTestCase { request.put("expr", new String[] {"${one_ref}"}); // expr is for streaming expressions, no replacement by default request.put("one_ref",new String[] {"one"}); request.put("three_ref",new String[] {"three"}); - @SuppressWarnings({"rawtypes"}) - Map expanded = MacroExpander.expand(request); - assertEquals("zero", ((String[])expanded.get("fq"))[0]); - assertEquals("one", ((String[])expanded.get("fq"))[1]); - assertEquals("two", ((String[]) expanded.get("fq"))[2]); - assertEquals("three", ((String[]) expanded.get("fq"))[3]); + Map expanded = MacroExpander.expand(request); + assertEquals("zero", expanded.get("fq")[0]); + assertEquals("one", expanded.get("fq")[1]); + assertEquals("two", expanded.get("fq")[2]); + assertEquals("three", expanded.get("fq")[3]); - assertEquals("${one_ref}", ((String[])expanded.get("expr"))[0]); + assertEquals("${one_ref}", expanded.get("expr")[0]); } @Test @@ -143,15 +142,36 @@ public class TestMacroExpander extends SolrTestCase { String oldVal = System.getProperty("StreamingExpressionMacros","false"); System.setProperty("StreamingExpressionMacros", "true"); try { - @SuppressWarnings({"rawtypes"}) - Map expanded = MacroExpander.expand(request); - assertEquals("zero", ((String[])expanded.get("fq"))[0]); - assertEquals("one", ((String[])expanded.get("fq"))[1]); - assertEquals("two", ((String[]) expanded.get("fq"))[2]); - assertEquals("three", ((String[]) expanded.get("fq"))[3]); - assertEquals("one", ((String[])expanded.get("expr"))[0]); + Map expanded = MacroExpander.expand(request); + assertEquals("zero", expanded.get("fq")[0]); + assertEquals("one", expanded.get("fq")[1]); + assertEquals("two", expanded.get("fq")[2]); + assertEquals("three", expanded.get("fq")[3]); + assertEquals("one", expanded.get("expr")[0]); } finally { System.setProperty("StreamingExpressionMacros", oldVal); } } + + @Test + public void testUnbalanced() { // SOLR-13181 + final Map request = Collections.singletonMap("answer", new String[]{ "42" }); + final MacroExpander meSkipOnMissingParams = new MacroExpander(request); + final MacroExpander meFailOnMissingParams = new MacroExpander(request, true); + assertEquals("${noClose", meSkipOnMissingParams.expand("${noClose")); + assertEquals("42 ${noClose", meSkipOnMissingParams.expand("${answer} ${noClose")); + assertEquals("42 ${noClose fooBar", meSkipOnMissingParams.expand("${answer} ${noClose fooBar")); + assertNull(meFailOnMissingParams.expand("${noClose")); + assertNull(meFailOnMissingParams.expand("${answer} ${noClose")); + assertNull(meFailOnMissingParams.expand("${answer} ${noClose fooBar")); + + assertEquals("${${b}}", meSkipOnMissingParams.expand("${${b}}")); + assertNull(meFailOnMissingParams.expand("${${b}}")); + + // Does not register as a syntax failure, although may subjectively look like it. + // Consequently, the expression is replaced with nothing in default mode. + // It'd be nice if there was a mode to leave un-resolved macros as-is when they don't resolve. + assertEquals("preamble ", meSkipOnMissingParams.expand("preamble ${exp${bad}")); + assertNull(meFailOnMissingParams.expand("preamble ${exp${bad}")); + } }