Merge pull request #33 from jglick/RejectedAccessException-JENKINS-31701

[JENKINS-31701] Misclassification of a method taking long and being passed an int
jenkinsci · Dec 16, 2015 · af0b1ae · af0b1ae
2 parents bad55dc + 200b118
commit af0b1ae
Show file tree

Hide file tree

Showing 3 changed files with 51 additions and 1 deletion.
diff --git a/...main/java/org/jenkinsci/plugins/scriptsecurity/sandbox/groovy/GroovyCallSiteSelector.java b/...main/java/org/jenkinsci/plugins/scriptsecurity/sandbox/groovy/GroovyCallSiteSelector.java
@@ -63,7 +63,7 @@ private static boolean matches(@Nonnull Class<?>[] parameterTypes, @Nonnull Obje
             if (
                     parameterTypes[i].isPrimitive()
                     && parameters[i] != null
-                    && ClassUtils.primitiveToWrapper(parameterTypes[i]).isInstance(parameters[i])
+                    && isInstancePrimitive(ClassUtils.primitiveToWrapper(parameterTypes[i]), parameters[i])
             ) {
                 // Groovy passes primitive values as objects (for example, passes 0 as Integer(0))
                 // The prior test fails as int.class.isInstance(new Integer(0)) returns false.
@@ -80,6 +80,29 @@ private static boolean matches(@Nonnull Class<?>[] parameterTypes, @Nonnull Obje
         return true;
     }
 
+    /**
+     * {@link Class#isInstance} extended to handle some important cases of primitive types.
+     */
+    private static boolean isInstancePrimitive(@Nonnull Class<?> type, @Nonnull Object instance) {
+        if (type.isInstance(instance)) {
+            return true;
+        }
+        // https://docs.oracle.com/javase/specs/jls/se8/html/jls-5.html#jls-5.1.2
+        if (instance instanceof Number) {
+            if (type == Long.class && instance instanceof Integer) {
+                return true; // widening
+            }
+            if (type == Integer.class && instance instanceof Long) {
+                Long n = (Long) instance;
+                if (n >= Integer.MIN_VALUE && n <= Integer.MAX_VALUE) {
+                    return true; // safe narrowing
+                }
+            }
+            // TODO etc. for other conversions if they ever come up
+        }
+        return false;
+    }
+
     /**
      * Looks up the most general possible definition of a given method call.
      * Preferentially searches for compatible definitions in supertypes.
@@ -166,6 +189,7 @@ private static void visitTypes(@Nonnull Set<Class<?>> types, @Nonnull Class<?> c
         types.add(c);
     }
 
+    // TODO nowhere close to implementing http://docs.oracle.com/javase/specs/jls/se8/html/jls-15.html#jls-15.12.2.5
     private static boolean isMoreSpecific(Method more, Method less) {
         Class<?>[] moreParams = more.getParameterTypes();
         Class<?>[] lessParams = less.getParameterTypes();
@@ -178,6 +202,11 @@ private static boolean isMoreSpecific(Method more, Method less) {
             } else if (lessParam.isAssignableFrom(moreParam)) {
                 return true;
             }
+            if (moreParam == Long.class && lessParam == Integer.class) {
+                return false;
+            } else if (moreParam == Integer.class && lessParam == Long.class) {
+                return true;
+            }
         }
         // Incomparable. Arbitrarily pick one of them.
         return more.toString().compareTo(less.toString()) > 0;

diff --git a/.../java/org/jenkinsci/plugins/scriptsecurity/sandbox/groovy/GroovyCallSiteSelectorTest.java b/.../java/org/jenkinsci/plugins/scriptsecurity/sandbox/groovy/GroovyCallSiteSelectorTest.java
@@ -58,4 +58,17 @@ public class GroovyCallSiteSelectorTest {
         assertEquals(GString.class.getMethod("getStrings"), GroovyCallSiteSelector.method(gString, "getStrings", new Object[0]));
     }
 
+    @Issue("JENKINS-31701")
+    @Test public void primitives() throws Exception {
+        assertEquals(Primitives.class.getMethod("m1", long.class), GroovyCallSiteSelector.staticMethod(Primitives.class, "m1", new Object[] {Long.MAX_VALUE}));
+        assertEquals(Primitives.class.getMethod("m1", long.class), GroovyCallSiteSelector.staticMethod(Primitives.class, "m1", new Object[] {99}));
+        assertEquals(Primitives.class.getMethod("m2", long.class), GroovyCallSiteSelector.staticMethod(Primitives.class, "m2", new Object[] {Long.MAX_VALUE}));
+        assertEquals(Primitives.class.getMethod("m2", int.class), GroovyCallSiteSelector.staticMethod(Primitives.class, "m2", new Object[] {99}));
+    }
+    public static class Primitives {
+        public static void m1(long x) {}
+        public static void m2(int x) {}
+        public static void m2(long x) {}
+    }
+
 }
diff --git a/...test/java/org/jenkinsci/plugins/scriptsecurity/sandbox/groovy/SandboxInterceptorTest.java b/...test/java/org/jenkinsci/plugins/scriptsecurity/sandbox/groovy/SandboxInterceptorTest.java
@@ -491,6 +491,14 @@ public Object invokeMethod(String name, Object args) {
         assertEvaluate(new GenericWhitelist(), true, "Calendar.instance.get(Calendar.DAY_OF_MONTH) < 32");
     }
 
+    @Issue("JENKINS-31701")
+    @Test public void primitiveWidening() throws Exception {
+        assertEvaluate(new AnnotatedWhitelist(), 4L, SandboxInterceptorTest.class.getName() + ".usePrimitive(2)");
+    }
+    @Whitelisted public static long usePrimitive(long x) {
+        return x + 2;
+    }
+
     private static void assertEvaluate(Whitelist whitelist, final Object expected, final String script) {
         final GroovyShell shell = new GroovyShell(GroovySandbox.createSecureCompilerConfiguration());
         Object actual = GroovySandbox.run(shell.parse(script), whitelist);