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

Nondeterministic selection of matching constructor

    XMLWordPrintable

    Details

    • 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

            Hide
            scm_issue_link SCM/JIRA link daemon added a comment -

            Code changed in jenkins
            User: Jesse Glick
            Path:
            src/main/java/org/jenkinsci/plugins/scriptsecurity/sandbox/groovy/GroovyCallSiteSelector.java
            src/test/java/org/jenkinsci/plugins/scriptsecurity/sandbox/groovy/GroovyCallSiteSelectorTest.java
            http://jenkins-ci.org/commit/script-security-plugin/f73fce4cefeea537a62670e63494411a25b471ca
            Log:
            [FIXED JENKINS-45117] Apply specificity comparisons to constructors, not just methods.

            Show
            scm_issue_link SCM/JIRA link daemon added a comment - Code changed in jenkins User: Jesse Glick Path: src/main/java/org/jenkinsci/plugins/scriptsecurity/sandbox/groovy/GroovyCallSiteSelector.java src/test/java/org/jenkinsci/plugins/scriptsecurity/sandbox/groovy/GroovyCallSiteSelectorTest.java http://jenkins-ci.org/commit/script-security-plugin/f73fce4cefeea537a62670e63494411a25b471ca Log: [FIXED JENKINS-45117] Apply specificity comparisons to constructors, not just methods.

              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: