[FIXED JENKINS-25524] Wrong overload of PrintWriter.print was sometim…

…es being selected, at random.
jenkinsci · Nov 10, 2014 · a2c5625 · a2c5625
1 parent 269acc8
commit a2c5625
Show file tree

Hide file tree

Showing 3 changed files with 39 additions and 5 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
@@ -24,6 +24,7 @@
 
 package org.jenkinsci.plugins.scriptsecurity.sandbox.groovy;
 
+import com.google.common.primitives.Primitives;
 import groovy.lang.GString;
 import java.lang.reflect.Constructor;
 import java.lang.reflect.Field;
@@ -88,11 +89,17 @@ private static boolean matches(@Nonnull Class<?>[] parameterTypes, @Nonnull Obje
      */
     public static @CheckForNull Method method(@Nonnull Object receiver, @Nonnull String method, @Nonnull Object[] args) {
         for (Class<?> c : types(receiver)) {
+            Method candidate = null;
             for (Method m : c.getDeclaredMethods()) {
                 if (m.getName().equals(method) && matches(m.getParameterTypes(), args)) {
-                    return m;
+                    if (candidate == null || isMoreSpecific(m, candidate)) {
+                        candidate = m;
+                    }
                 }
             }
+            if (candidate != null) {
+                return candidate;
+            }
         }
         return null;
     }
@@ -108,13 +115,15 @@ private static boolean matches(@Nonnull Class<?>[] parameterTypes, @Nonnull Obje
 
     public static @CheckForNull Method staticMethod(@Nonnull Class<?> receiver, @Nonnull String method, @Nonnull Object[] args) {
         // TODO should we check for inherited static calls?
+        Method candidate = null;
         for (Method m : receiver.getDeclaredMethods()) {
             if (m.getName().equals(method) && matches(m.getParameterTypes(), args)) {
-                // TODO should we issue an error if multiple overloads match? or try to find the “most specific”?
-                return m;
+                if (candidate == null || isMoreSpecific(m, candidate)) {
+                    candidate = m;
+                }
             }
         }
-        return null;
+        return candidate;
     }
 
     public static @CheckForNull Field field(@Nonnull Object receiver, @Nonnull String field) {
@@ -154,6 +163,22 @@ private static void visitTypes(@Nonnull Set<Class<?>> types, @Nonnull Class<?> c
         types.add(c);
     }
 
+    private static boolean isMoreSpecific(Method more, Method less) {
+        Class<?>[] moreParams = more.getParameterTypes();
+        Class<?>[] lessParams = less.getParameterTypes();
+        assert moreParams.length == lessParams.length;
+        for (int i = 0; i < moreParams.length; i++) {
+            Class<?> moreParam = Primitives.wrap(moreParams[i]);
+            Class<?> lessParam = Primitives.wrap(lessParams[i]);
+            if (moreParam.isAssignableFrom(lessParam)) {
+                return false;
+            } else if (lessParam.isAssignableFrom(moreParam)) {
+                return true;
+            }
+        }
+        throw new IllegalStateException(more + " and " + less + " seem incomparable");
+    }
+
     private GroovyCallSiteSelector() {}
 
 }
diff --git a/.../java/org/jenkinsci/plugins/scriptsecurity/sandbox/groovy/GroovyCallSiteSelectorTest.java b/.../java/org/jenkinsci/plugins/scriptsecurity/sandbox/groovy/GroovyCallSiteSelectorTest.java
@@ -24,6 +24,8 @@
 
 package org.jenkinsci.plugins.scriptsecurity.sandbox.groovy;
 
+import com.google.common.io.NullOutputStream;
+import java.io.PrintWriter;
 import java.lang.reflect.Method;
 import org.jenkinsci.plugins.scriptsecurity.sandbox.whitelists.EnumeratingWhitelistTest;
 import static org.junit.Assert.*;
@@ -38,4 +40,11 @@ public class GroovyCallSiteSelectorTest {
         assertEquals("array cast", m, GroovyCallSiteSelector.method(new EnumeratingWhitelistTest.C(), "m", new Object[] {new String[] {"a", "b"}}));
     }
 
+    @Test public void overloads() throws Exception {
+        PrintWriter receiver = new PrintWriter(new NullOutputStream());
+        assertEquals(PrintWriter.class.getMethod("print", Object.class), GroovyCallSiteSelector.method(receiver, "print", new Object[] {new Object()}));
+        assertEquals(PrintWriter.class.getMethod("print", String.class), GroovyCallSiteSelector.method(receiver, "print", new Object[] {"message"}));
+        assertEquals(PrintWriter.class.getMethod("print", int.class), GroovyCallSiteSelector.method(receiver, "print", new Object[] {42}));
+    }
+
 }
diff --git a/...test/java/org/jenkinsci/plugins/scriptsecurity/sandbox/groovy/SandboxInterceptorTest.java b/...test/java/org/jenkinsci/plugins/scriptsecurity/sandbox/groovy/SandboxInterceptorTest.java
@@ -312,7 +312,7 @@ private Unsafe() {}
             @Override public String call() throws Exception {
                 return t.make(new HashMap<String,Object>(Collections.singletonMap("aspect", "CRUEL"))).toString();
             }
-        }, new StaticWhitelist("method java.lang.String toLowerCase", "method java.io.PrintWriter print java.lang.Object")));
+        }, new StaticWhitelist("method java.lang.String toLowerCase", "method java.io.PrintWriter print java.lang.String")));
     }
 
     @Test public void selfProperties() throws Exception {