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

Nondeterministic selection of matching constructor

    Details

    • Type: Bug
    • Status: Resolved
    • Priority: Critical
    • Resolution: Fixed
    • Component/s: script-security-plugin
    • Labels:
      None
    • Similar Issues:

      Description

      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.

        Attachments

          Issue Links

            Activity

              People

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

                Dates

                • Created:
                  Updated:
                  Resolved: