[FIXED JENKINS-48501] Null-safety in varargs->array check

Added some more null checks in the vicinity just to be safe, but the main thing here is that we needed to avoid an NPE *and* treat a null arg as if it were of the target method's trailing array parameter's component type, since, well, null is every type.
jenkinsci · Dec 12, 2017 · 1e71557 · 1e71557
1 parent 9074396
commit 1e71557
Show file tree

Hide file tree

Showing 2 changed files with 22 additions and 9 deletions.
diff --git a/...main/java/org/jenkinsci/plugins/scriptsecurity/sandbox/groovy/GroovyCallSiteSelector.java b/...main/java/org/jenkinsci/plugins/scriptsecurity/sandbox/groovy/GroovyCallSiteSelector.java
@@ -193,15 +193,17 @@ private static boolean isInstancePrimitive(@Nonnull Class<?> type, @Nonnull Obje
         return findMatchingMethod(receiver, method, args);
     }
 
-    private static Method findMatchingMethod(Class<?> receiver, String method, Object[] args) {
+    private static Method findMatchingMethod(@Nonnull Class<?> receiver, @Nonnull String method, @Nonnull Object[] args) {
         Method candidate = null;
 
         for (Method m : receiver.getDeclaredMethods()) {
-            boolean isVarArgs = isVarArgsMethod(m, args);
-            if (m.getName().equals(method) && (matches(m.getParameterTypes(), args, isVarArgs))) {
-                if (candidate == null || isMoreSpecific(m, m.getParameterTypes(), isVarArgs, candidate,
-                        candidate.getParameterTypes(), isVarArgsMethod(candidate, args))) {
-                    candidate = m;
+            if (m != null) {
+                boolean isVarArgs = isVarArgsMethod(m, args);
+                if (m.getName().equals(method) && (matches(m.getParameterTypes(), args, isVarArgs))) {
+                    if (candidate == null || isMoreSpecific(m, m.getParameterTypes(), isVarArgs, candidate,
+                            candidate.getParameterTypes(), isVarArgsMethod(candidate, args))) {
+                        candidate = m;
+                    }
                 }
             }
         }
@@ -211,7 +213,7 @@ private static Method findMatchingMethod(Class<?> receiver, String method, Objec
     /**
      * Emulates, with some tweaks, {@link org.codehaus.groovy.reflection.ParameterTypes#isVargsMethod(Object[])}
      */
-    private static boolean isVarArgsMethod(Method m, Object[] args) {
+    private static boolean isVarArgsMethod(@Nonnull Method m, @Nonnull Object[] args) {
         if (m.isVarArgs()) {
             return true;
         }
@@ -226,9 +228,9 @@ private static boolean isVarArgsMethod(Method m, Object[] args) {
         // If there are more arguments than parameter types and the last parameter type is an array, we may be vargy.
         if (paramTypes[lastIndex].isArray() && args.length > paramTypes.length) {
             Class<?> lastClass = paramTypes[lastIndex].getComponentType();
-            // Check each possible vararg to see if it can be cast to the array's component type. If not, we're not vargy.
+            // Check each possible vararg to see if it can be cast to the array's component type or is null. If not, we're not vargy.
             for (int i = lastIndex; i < args.length; i++) {
-                if (!lastClass.isAssignableFrom(args[i].getClass())) {
+                if (args[i] != null && !lastClass.isAssignableFrom(args[i].getClass())) {
                     return false;
                 }
             }

diff --git a/...test/java/org/jenkinsci/plugins/scriptsecurity/sandbox/groovy/SandboxInterceptorTest.java b/...test/java/org/jenkinsci/plugins/scriptsecurity/sandbox/groovy/SandboxInterceptorTest.java
@@ -1011,4 +1011,15 @@ public void castAsFile() throws Exception {
         assertRejected(new GenericWhitelist(), "new java.io.File java.lang.String",
                 "def s = []; ('/tmp/foo' as File).each { s << it }\n");
     }
+
+    @Issue("JENKINS-48501")
+    @Test
+    public void nullInVarArgsAsArray() throws Exception {
+        String script = "def TEST_FMT = 'a:%s b:%s c:%s d:%s'\n" +
+                "String s = sprintf(TEST_FMT, null, '2', '3', '4')\n" +
+                "return s\n";
+        assertEvaluate(new StaticWhitelist("staticMethod org.codehaus.groovy.runtime.DefaultGroovyMethods sprintf java.lang.Object java.lang.String java.lang.Object[]"),
+                "a:null b:2 c:3 d:4",
+                script);
+    }
 }