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

Dubious use of String.toUpperCase()

    XMLWordPrintable

    Details

    • Type: Bug
    • Status: Reopened (View Workflow)
    • Priority: Major
    • Resolution: Unresolved
    • Component/s: core
    • Labels:
      None
    • Similar Issues:

      Description

      StringParameterValue.buildEnvVars says

      env.put(name.toUpperCase(),value);

      Say the user specified 'includedTests' in the job configuration and used an Ant script:

      <property env="env."/>
      <junit ... includes="${env.includedTests}"/>

      First of all, this will not work because only ${env.INCLUDEDTESTS} will be defined. It is nice that name.html says this will happen, but you would have to click the help button to see this unexpected behavior.

      Second, if you happen to run the Hudson server in Turkey, even that won't work because the environment variable will be ${env.─░NCLUDEDTESTS} due to the locale-sensitive upcasing!

      At a minimum, all usages of String.toUpper/LowerCase() in Hudson should be reviewed to see if passing Locale.ENGLISH would be more appropriate.

      In the case of *ParameterValue.buildEnvVars, it should be reconsidered whether automatic upcasing is even desirable; surely it is clearer to use an uppercase variable name in the job config if that is what you want to see during the build. (For compatibility, you could set both the upcased string and the original string, and remove the note about upcasing from the help text.)

        Attachments

          Issue Links

            Activity

            jglick Jesse Glick created issue -
            Hide
            scm_issue_link SCM/JIRA link daemon added a comment -

            Code changed in hudson
            User: : kohsuke
            Path:
            trunk/hudson/main/core/src/main/java/hudson/EnvVars.java
            trunk/hudson/main/core/src/main/java/hudson/model/BooleanParameterValue.java
            trunk/hudson/main/core/src/main/java/hudson/model/JobParameterValue.java
            trunk/hudson/main/core/src/main/java/hudson/model/ParameterValue.java
            trunk/hudson/main/core/src/main/java/hudson/model/RunParameterValue.java
            trunk/hudson/main/core/src/main/java/hudson/model/StringParameterValue.java
            trunk/www/changelog.html
            http://jenkins-ci.org/commit/26640
            Log:
            [FIXED JENKINS-5391] in 1.344. we should be case insensitive but case preserving.

            Show
            scm_issue_link SCM/JIRA link daemon added a comment - Code changed in hudson User: : kohsuke Path: trunk/hudson/main/core/src/main/java/hudson/EnvVars.java trunk/hudson/main/core/src/main/java/hudson/model/BooleanParameterValue.java trunk/hudson/main/core/src/main/java/hudson/model/JobParameterValue.java trunk/hudson/main/core/src/main/java/hudson/model/ParameterValue.java trunk/hudson/main/core/src/main/java/hudson/model/RunParameterValue.java trunk/hudson/main/core/src/main/java/hudson/model/StringParameterValue.java trunk/www/changelog.html http://jenkins-ci.org/commit/26640 Log: [FIXED JENKINS-5391] in 1.344. we should be case insensitive but case preserving.
            scm_issue_link SCM/JIRA link daemon made changes -
            Field Original Value New Value
            Resolution Fixed [ 1 ]
            Status Open [ 1 ] Resolved [ 5 ]
            Hide
            jglick Jesse Glick added a comment -

            Note that there remain a number of suspect usages of String.toUpper/LowerCase in Hudson code; for example, in LogRecorderManager:

            @QueryParameter String level;
            // ...
            lv = Level.parse(level.toUpperCase());

            will throw an exception if ?level=info is passed to a server running in Turkish (or Azeri) locale.

            (I have lobbied to get these two methods @Deprecated since nearly all calls turn out to be incorrect on inspection, and the remainder can more clearly be expressed as to*Case(Locale.getDefault()); no one seems to be listening so far.)

            Show
            jglick Jesse Glick added a comment - Note that there remain a number of suspect usages of String.toUpper/LowerCase in Hudson code; for example, in LogRecorderManager: @QueryParameter String level; // ... lv = Level.parse(level.toUpperCase()); will throw an exception if ?level=info is passed to a server running in Turkish (or Azeri) locale. (I have lobbied to get these two methods @Deprecated since nearly all calls turn out to be incorrect on inspection, and the remainder can more clearly be expressed as to*Case(Locale.getDefault()); no one seems to be listening so far.)
            Hide
            scm_issue_link SCM/JIRA link daemon added a comment -

            Code changed in hudson
            User: : kohsuke
            Path:
            trunk/hudson/main/core/src/main/java/hudson/logging/LogRecorder.java
            trunk/hudson/main/core/src/main/java/hudson/logging/LogRecorderManager.java
            trunk/hudson/main/core/src/main/java/hudson/model/FingerprintMap.java
            trunk/hudson/main/core/src/main/java/hudson/model/Hudson.java
            trunk/hudson/main/core/src/main/java/hudson/model/MultiStageTimeSeries.java
            trunk/hudson/main/core/src/main/java/hudson/node_monitors/DiskSpaceMonitorDescriptor.java
            trunk/hudson/main/core/src/main/java/hudson/tools/JDKInstaller.java
            trunk/hudson/main/core/src/main/java/hudson/util/FormFieldValidator.java
            trunk/hudson/main/core/src/main/java/hudson/util/FormValidation.java
            trunk/hudson/main/core/src/main/java/hudson/util/TableNestChecker.java
            trunk/hudson/main/core/src/main/java/hudson/util/VersionNumber.java
            trunk/hudson/main/remoting/src/main/java/hudson/remoting/Launcher.java
            trunk/hudson/main/test/src/main/java/org/jvnet/hudson/test/HudsonPageCreator.java
            trunk/hudson/main/test/src/main/java/org/jvnet/hudson/test/recipes/PresetData.java
            trunk/hudson/main/test/src/test/java/hudson/bugs/JnlpAccessWithSecuredHudsonTest.java
            http://jenkins-ci.org/commit/26650
            Log:
            JENKINS-5391 Fixed the infamous Turkish bug.

            Show
            scm_issue_link SCM/JIRA link daemon added a comment - Code changed in hudson User: : kohsuke Path: trunk/hudson/main/core/src/main/java/hudson/logging/LogRecorder.java trunk/hudson/main/core/src/main/java/hudson/logging/LogRecorderManager.java trunk/hudson/main/core/src/main/java/hudson/model/FingerprintMap.java trunk/hudson/main/core/src/main/java/hudson/model/Hudson.java trunk/hudson/main/core/src/main/java/hudson/model/MultiStageTimeSeries.java trunk/hudson/main/core/src/main/java/hudson/node_monitors/DiskSpaceMonitorDescriptor.java trunk/hudson/main/core/src/main/java/hudson/tools/JDKInstaller.java trunk/hudson/main/core/src/main/java/hudson/util/FormFieldValidator.java trunk/hudson/main/core/src/main/java/hudson/util/FormValidation.java trunk/hudson/main/core/src/main/java/hudson/util/TableNestChecker.java trunk/hudson/main/core/src/main/java/hudson/util/VersionNumber.java trunk/hudson/main/remoting/src/main/java/hudson/remoting/Launcher.java trunk/hudson/main/test/src/main/java/org/jvnet/hudson/test/HudsonPageCreator.java trunk/hudson/main/test/src/main/java/org/jvnet/hudson/test/recipes/PresetData.java trunk/hudson/main/test/src/test/java/hudson/bugs/JnlpAccessWithSecuredHudsonTest.java http://jenkins-ci.org/commit/26650 Log: JENKINS-5391 Fixed the infamous Turkish bug.
            Hide
            mcrooney mcrooney added a comment -

            This actually breaks any existing parameterized builds which have different casing, so the changelog should probably have a strongly emphasized warning that upgrading has the possibility of breaking parameterized builds.

            For example, say we have parameters "foo" and "bar", and are referring to them in our jobs via the environment variables "FOO" and "BAR" (because we have to because of the forced upper casing). After upgrading, these environment variables no longer exist and the jobs no longer run. I can of course change the casing to match either way but, it would have been nice to know before upgrading that I might have broken jobs on my hands so I can fix them before they all blow up

            Show
            mcrooney mcrooney added a comment - This actually breaks any existing parameterized builds which have different casing, so the changelog should probably have a strongly emphasized warning that upgrading has the possibility of breaking parameterized builds. For example, say we have parameters "foo" and "bar", and are referring to them in our jobs via the environment variables "FOO" and "BAR" (because we have to because of the forced upper casing). After upgrading, these environment variables no longer exist and the jobs no longer run. I can of course change the casing to match either way but, it would have been nice to know before upgrading that I might have broken jobs on my hands so I can fix them before they all blow up
            Hide
            jglick Jesse Glick added a comment -

            Hence my suggestion that "for compatibility, you could set both the upcased string and the original string".

            Given the new usage of EnvVars (and that its expand method should be case-insensitive already), probably this would be accomplished where ProcessBuilder.environment() is populated from EnvVars - in Launcher and Proc and perhaps elsewhere.

            Show
            jglick Jesse Glick added a comment - Hence my suggestion that "for compatibility, you could set both the upcased string and the original string". Given the new usage of EnvVars (and that its expand method should be case-insensitive already), probably this would be accomplished where ProcessBuilder.environment() is populated from EnvVars - in Launcher and Proc and perhaps elsewhere.
            Hide
            mcrooney mcrooney added a comment -

            Still seems to be broken on 1.345, I have a failing parameterized build and doing an 'env' in a build step shows only the lowercase versions defined.

            Show
            mcrooney mcrooney added a comment - Still seems to be broken on 1.345, I have a failing parameterized build and doing an 'env' in a build step shows only the lowercase versions defined.
            mcrooney mcrooney made changes -
            Resolution Fixed [ 1 ]
            Status Resolved [ 5 ] Reopened [ 4 ]
            vkodocha vkodocha made changes -
            Link This issue is blocking JENKINS-5843 [ JENKINS-5843 ]
            Hide
            vkodocha vkodocha added a comment -

            I've created the issue 5843 which is basically a duplicate of this here.

            I've seen that the backwards compatibility should be solved since version 1.345. And I also looked at the code of the current version. The code looks fine but it does not work for me.

            I have tested this with a clean install of Hudson 1.351, created a new job with one parameter with a lowercase name, and one build step to show all environment vars. I tested this on Mac OS X 10.6 and on a Vista machine with the same result for both platforms. Only the lowercase variable is set, the uppercase variant is just missing.

            On windows it seams not to be working as the environments vars seams to be ignoring case. Meaning a set hello=5 and then a set HELLO=10 will result in a variable named hello=10 to be set. But on mac this is not the case so I don't know why it is not working.
            (I don't have a problem with it not woking under Windows as I use Mac but I just wanted to verify that it's not a mac specific problem).

            Show
            vkodocha vkodocha added a comment - I've created the issue 5843 which is basically a duplicate of this here. I've seen that the backwards compatibility should be solved since version 1.345. And I also looked at the code of the current version. The code looks fine but it does not work for me. I have tested this with a clean install of Hudson 1.351, created a new job with one parameter with a lowercase name, and one build step to show all environment vars. I tested this on Mac OS X 10.6 and on a Vista machine with the same result for both platforms. Only the lowercase variable is set, the uppercase variant is just missing. On windows it seams not to be working as the environments vars seams to be ignoring case. Meaning a set hello=5 and then a set HELLO=10 will result in a variable named hello=10 to be set. But on mac this is not the case so I don't know why it is not working. (I don't have a problem with it not woking under Windows as I use Mac but I just wanted to verify that it's not a mac specific problem).
            Hide
            slide_o_mix Alex Earl added a comment -

            Is this still an issue?

            Show
            slide_o_mix Alex Earl added a comment - Is this still an issue?
            Hide
            danielbeck Daniel Beck added a comment -

            In a different way, yes:
            https://github.com/jenkinsci/jenkins/blob/d8af38b7e3367f3988ceb1d9e15b2fed4efd7971/core/src/main/java/hudson/model/StringParameterValue.java#L55...L57

            It simply defines both.

            Which is probably hilarious on the case of parameter names such as "User" or "Home".

            Related insanity is JENKINS-16255.

            Show
            danielbeck Daniel Beck added a comment - In a different way, yes: https://github.com/jenkinsci/jenkins/blob/d8af38b7e3367f3988ceb1d9e15b2fed4efd7971/core/src/main/java/hudson/model/StringParameterValue.java#L55...L57 It simply defines both. Which is probably hilarious on the case of parameter names such as "User" or "Home". Related insanity is JENKINS-16255 .
            rtyler R. Tyler Croy made changes -
            Workflow JNJira [ 135480 ] JNJira + In-Review [ 185940 ]

              People

              Assignee:
              Unassigned Unassigned
              Reporter:
              jglick Jesse Glick
              Votes:
              2 Vote for this issue
              Watchers:
              3 Start watching this issue

                Dates

                Created:
                Updated: