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

Dubious use of String.toUpperCase()

    Details

    • Type: Bug
    • Status: Reopened
    • Priority: Major
    • Resolution: Unresolved
    • Component/s: core
    • Labels:
      None

      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.)

        Issue Links

          Activity

          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.
          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.
          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).

            People

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

              Dates

              • Created:
                Updated: