SQL: integer parameter validation in string functions (elastic#63338)

* SQL: integer parameter validation in string functions (elastic#58923) In insert, locate, substring function, when argument `start` or `length` is greater than Integer.MAX_INT OR less then Integer.MIN_INT + 1 (note that `start` need to minus 1), it causes overflow and leads to unexpected results. * Add range checks for BinaryStringNumericProcessors - Add range checks for Left, Right, Repeat. - Minor refactorings on initial PR changes. Co-authored-by: yinanwu <[email protected]>
nailushapotnuru · Oct 15, 2020 · bf6dc58 · bf6dc58
1 parent 45a6023
commit bf6dc58
Show file tree

Hide file tree

Showing 9 changed files with 196 additions and 97 deletions.
diff --git a/...asticsearch/xpack/sql/expression/function/scalar/string/BinaryStringNumericProcessor.java b/...asticsearch/xpack/sql/expression/function/scalar/string/BinaryStringNumericProcessor.java
@@ -10,6 +10,7 @@
 import org.elasticsearch.xpack.ql.expression.gen.processor.Processor;
 import org.elasticsearch.xpack.sql.SqlIllegalArgumentException;
 import org.elasticsearch.xpack.sql.expression.function.scalar.string.BinaryStringNumericProcessor.BinaryStringNumericOperation;
+import org.elasticsearch.xpack.sql.util.Check;
 
 import java.io.IOException;
 import java.util.function.BiFunction;
@@ -19,7 +20,7 @@
  * second parameter as numeric and a string result.
  */
 public class BinaryStringNumericProcessor extends FunctionalEnumBinaryProcessor<String, Number, String, BinaryStringNumericOperation> {
-    
+
     public enum BinaryStringNumericOperation implements BiFunction<String, Number, String> {
         LEFT((s,c) -> {
             int i = c.intValue();
@@ -40,7 +41,7 @@ public enum BinaryStringNumericOperation implements BiFunction<String, Number, S
             if (i <= 0) {
                 return null;
             }
-            
+
             StringBuilder sb = new StringBuilder(s.length() * i);
             for (int j = 0; j < i; j++) {
                 sb.append(s);
@@ -51,7 +52,7 @@ public enum BinaryStringNumericOperation implements BiFunction<String, Number, S
         BinaryStringNumericOperation(BiFunction<String, Number, String> op) {
             this.op = op;
         }
-        
+
         private final BiFunction<String, Number, String> op;
 
         @Override
@@ -83,10 +84,9 @@ protected Object doProcess(Object left, Object right) {
         if (!(left instanceof String || left instanceof Character)) {
             throw new SqlIllegalArgumentException("A string/char is required; received [{}]", left);
         }
-        if (!(right instanceof Number)) {
-            throw new SqlIllegalArgumentException("A number is required; received [{}]", right);
-        }
+        // count can be negative, the case is handled by the code, but it must still be int-convertible
+        Check.isFixedNumberAndInRange(right, "count", (long) Integer.MIN_VALUE, (long) Integer.MAX_VALUE);
 
         return super.doProcess(left.toString(), right);
     }
-}
+}
diff --git a/...rg/elasticsearch/xpack/sql/expression/function/scalar/string/InsertFunctionProcessor.java b/...rg/elasticsearch/xpack/sql/expression/function/scalar/string/InsertFunctionProcessor.java
@@ -9,6 +9,7 @@
 import org.elasticsearch.common.io.stream.StreamOutput;
 import org.elasticsearch.xpack.ql.expression.gen.processor.Processor;
 import org.elasticsearch.xpack.sql.SqlIllegalArgumentException;
+import org.elasticsearch.xpack.sql.util.Check;
 
 import java.io.IOException;
 import java.util.Objects;
@@ -61,65 +62,59 @@ public static Object doProcess(Object input, Object start, Object length, Object
         if (start == null || length == null) {
             return input;
         }
-        if (!(start instanceof Number)) {
-            throw new SqlIllegalArgumentException("A number is required; received [{}]", start);
-        }
-        if (!(length instanceof Number)) {
-            throw new SqlIllegalArgumentException("A number is required; received [{}]", length);
-        }
-        if (((Number) length).intValue() < 0) {
-            throw new SqlIllegalArgumentException("A positive number is required for [length]; received [{}]", length);
-        }
+
+        Check.isFixedNumberAndInRange(start, "start", (long) Integer.MIN_VALUE + 1, (long) Integer.MAX_VALUE);
+        Check.isFixedNumberAndInRange(length, "length", 0L, (long) Integer.MAX_VALUE);
 
         int startInt = ((Number) start).intValue() - 1;
         int realStart = startInt < 0 ? 0 : startInt;
-        
+
         if (startInt > input.toString().length()) {
             return input;
         }
-        
+
         StringBuilder sb = new StringBuilder(input.toString());
         String replString = (replacement.toString());
 
         return sb.replace(realStart,
                 realStart + ((Number) length).intValue(),
                 replString).toString();
     }
-    
+
     @Override
     public boolean equals(Object obj) {
         if (this == obj) {
             return true;
         }
-        
+
         if (obj == null || getClass() != obj.getClass()) {
             return false;
         }
-        
+
         InsertFunctionProcessor other = (InsertFunctionProcessor) obj;
         return Objects.equals(input(), other.input())
                 && Objects.equals(start(), other.start())
                 && Objects.equals(length(), other.length())
                 && Objects.equals(replacement(), other.replacement());
     }
-    
+
     @Override
     public int hashCode() {
         return Objects.hash(input(), start(), length(), replacement());
     }
-    
+
     public Processor input() {
         return input;
     }
-    
+
     public Processor start() {
         return start;
     }
-    
+
     public Processor length() {
         return length;
     }
-    
+
     public Processor replacement() {
         return replacement;
     }

diff --git a/...rg/elasticsearch/xpack/sql/expression/function/scalar/string/LocateFunctionProcessor.java b/...rg/elasticsearch/xpack/sql/expression/function/scalar/string/LocateFunctionProcessor.java
@@ -9,6 +9,7 @@
 import org.elasticsearch.common.io.stream.StreamOutput;
 import org.elasticsearch.xpack.ql.expression.gen.processor.Processor;
 import org.elasticsearch.xpack.sql.SqlIllegalArgumentException;
+import org.elasticsearch.xpack.sql.util.Check;
 
 import java.io.IOException;
 import java.util.Objects;
@@ -52,51 +53,52 @@ public static Integer doProcess(Object pattern, Object input, Object start) {
         if (pattern == null) {
             return 0;
         }
-        
+
         if (!(pattern instanceof String || pattern instanceof Character)) {
             throw new SqlIllegalArgumentException("A string/char is required; received [{}]", pattern);
         }
-        if (start != null && !(start instanceof Number)) {
-            throw new SqlIllegalArgumentException("A number is required; received [{}]", start);
+
+        if (start != null) {
+            Check.isFixedNumberAndInRange(start, "start", (long) Integer.MIN_VALUE + 1, (long) Integer.MAX_VALUE);
         }
-        
+
         String stringInput = input instanceof Character ? input.toString() : (String) input;
         String stringPattern = pattern instanceof Character ? pattern.toString() : (String) pattern;
 
-        return Integer.valueOf(1 + (start != null ? 
+        return Integer.valueOf(1 + (start != null ?
                 stringInput.indexOf(stringPattern, ((Number) start).intValue() - 1)
                 : stringInput.indexOf(stringPattern)));
     }
-    
+
     @Override
     public boolean equals(Object obj) {
         if (this == obj) {
             return true;
         }
-        
+
         if (obj == null || getClass() != obj.getClass()) {
             return false;
         }
-        
+
         LocateFunctionProcessor other = (LocateFunctionProcessor) obj;
         return Objects.equals(pattern(), other.pattern())
                 && Objects.equals(input(), other.input())
                 && Objects.equals(start(), other.start());
     }
-    
+
     @Override
     public int hashCode() {
         return Objects.hash(pattern(), input(), start());
     }
-    
+
     public Processor pattern() {
         return pattern;
     }
-    
+
     public Processor input() {
         return input;
     }
-    
+
     public Processor start() {
         return start;
     }

diff --git a/...elasticsearch/xpack/sql/expression/function/scalar/string/SubstringFunctionProcessor.java b/...elasticsearch/xpack/sql/expression/function/scalar/string/SubstringFunctionProcessor.java
@@ -9,6 +9,7 @@
 import org.elasticsearch.common.io.stream.StreamOutput;
 import org.elasticsearch.xpack.ql.expression.gen.processor.Processor;
 import org.elasticsearch.xpack.sql.SqlIllegalArgumentException;
+import org.elasticsearch.xpack.sql.util.Check;
 
 import java.io.IOException;
 import java.util.Objects;
@@ -53,54 +54,48 @@ public static Object doProcess(Object input, Object start, Object length) {
         if (start == null || length == null) {
             return input;
         }
-        if (!(start instanceof Number)) {
-            throw new SqlIllegalArgumentException("A number is required; received [{}]", start);
-        }
-        if (!(length instanceof Number)) {
-            throw new SqlIllegalArgumentException("A number is required; received [{}]", length);
-        }
-        if (((Number) length).intValue() < 0) {
-            throw new SqlIllegalArgumentException("A positive number is required for [length]; received [{}]", length);
-        }
+
+        Check.isFixedNumberAndInRange(start, "start", (long) Integer.MIN_VALUE + 1, (long) Integer.MAX_VALUE);
+        Check.isFixedNumberAndInRange(length, "length", 0L, (long) Integer.MAX_VALUE);
 
         return StringFunctionUtils.substring(input instanceof Character ? input.toString() : (String) input,
                 ((Number) start).intValue() - 1, // SQL is 1-based when it comes to string manipulation
                 ((Number) length).intValue());
     }
-    
+
     protected Processor input() {
         return input;
     }
-    
+
     protected Processor start() {
         return start;
     }
-    
+
     protected Processor length() {
         return length;
     }
-    
+
     @Override
     public boolean equals(Object obj) {
         if (this == obj) {
             return true;
         }
-        
+
         if (obj == null || getClass() != obj.getClass()) {
             return false;
         }
-        
+
         SubstringFunctionProcessor other = (SubstringFunctionProcessor) obj;
         return Objects.equals(input(), other.input())
                 && Objects.equals(start(), other.start())
                 && Objects.equals(length(), other.length());
     }
-    
+
     @Override
     public int hashCode() {
         return Objects.hash(input(), start(), length());
     }
-    
+
 
     @Override
     public String getWriteableName() {

diff --git a/x-pack/plugin/sql/src/main/java/org/elasticsearch/xpack/sql/util/Check.java b/x-pack/plugin/sql/src/main/java/org/elasticsearch/xpack/sql/util/Check.java
@@ -9,7 +9,7 @@
 
 /**
  * Utility class used for checking various conditions at runtime, inside SQL (hence the specific exception) with
- * minimum amount of code 
+ * minimum amount of code
  */
 public abstract class Check {
 
@@ -36,4 +36,16 @@ public static void notNull(Object object, String message, Object... values) {
             throw new SqlIllegalArgumentException(message, values);
         }
     }
+
+    public static void isFixedNumberAndInRange(Object object, String objectName, Long from, Long to) {
+        if ((object instanceof Number) == false || object instanceof Float || object instanceof Double) {
+            throw new SqlIllegalArgumentException("A fixed point number is required for [{}]; received [{}]", objectName,
+                object.getClass().getTypeName());
+        }
+        Long longValue = ((Number) object).longValue();
+        if (longValue < from || longValue > to) {
+            throw new SqlIllegalArgumentException("[{}] out of the allowed range [{}, {}], received [{}]", objectName, from, to,
+                longValue);
+        }
+    }
 }