Uploaded image for project: 'Jenkins'
  1. Jenkins
  2. JENKINS-45117

Nondeterministic selection of matching constructor

XMLWordPrintable

      GroovyCallSiteSelector.isMoreSpecific is supposed to select the most applicable method for a given call. Yet this only applies to Method selection. Constructor selection just picks the "first" matching constructor, which is nondeterministic, so that for example a call to new hudson.EnvVars() given the current whitelist entry new hudson.EnvVars may or may not succeed, depending on whether you use the plugin as is or with the following applied:

      diff --git a/src/main/java/org/jenkinsci/plugins/scriptsecurity/sandbox/groovy/GroovyCallSiteSelector.java b/src/main/java/org/jenkinsci/plugins/scriptsecurity/sandbox/groovy/GroovyCallSiteSelector.java
index cf253de..62e5e79 100644
--- a/src/main/java/org/jenkinsci/plugins/scriptsecurity/sandbox/groovy/GroovyCallSiteSelector.java
+++ b/src/main/java/org/jenkinsci/plugins/scriptsecurity/sandbox/groovy/GroovyCallSiteSelector.java
@@ -30,11 +30,13 @@ import java.lang.reflect.Array;
 import java.lang.reflect.Constructor;
 import java.lang.reflect.Field;
 import java.lang.reflect.Method;
+import java.util.Arrays;
 import java.util.LinkedHashSet;
 import java.util.Map;
 import java.util.Set;
 import javax.annotation.CheckForNull;
 import javax.annotation.Nonnull;
+import org.apache.commons.lang.ArrayUtils;
 import org.apache.commons.lang.ClassUtils;
 
 /**
@@ -159,7 +161,9 @@ class GroovyCallSiteSelector {
     }
 
     public static @CheckForNull Constructor<?> constructor(@Nonnull Class<?> receiver, @Nonnull Object[] args) {
-        for (Constructor<?> c : receiver.getDeclaredConstructors()) {
+        Constructor<?>[] constructors = receiver.getDeclaredConstructors();
+        ArrayUtils.reverse(constructors);
+        for (Constructor<?> c : constructors) {
             if (matches(c.getParameterTypes(), args, c.isVarArgs())) {
                 return c;
             }
@@ -169,7 +173,7 @@ class GroovyCallSiteSelector {
         // Also note that this logic is derived from how Groovy itself decides to use the magic Map constructor, at
         // MetaClassImpl#invokeConstructor(Class, Object[]).
         if (args.length == 1 && args[0] instanceof Map) {
-            for (Constructor<?> c : receiver.getDeclaredConstructors()) {
+            for (Constructor<?> c : constructors) {
                 if (c.getParameterTypes().length == 0 && !c.isVarArgs()) {
                     return c;
                 }

      In this case we would want the non-varargs overload (the one currently whitelisted) to be preferred.

            jglick Jesse Glick
            jglick Jesse Glick
            Votes:
            0 Vote for this issue
            Watchers:
            2 Start watching this issue

              Created:
              Updated:
              Resolved: