Commons CLI
  1. Commons CLI
  2. CLI-162

infinite loop in the wrapping code of HelpFormatter

    Details

    • Type: Bug Bug
    • Status: Closed
    • Priority: Major Major
    • Resolution: Fixed
    • Affects Version/s: 1.1
    • Fix Version/s: 1.2
    • Component/s: Help formatter
    • Labels:
      None

      Description

      If there is not enough space to display a word on a single line, HelpFormatter goes into a infinite loops until the JVM crashes with an OutOfMemoryError.

      Test case:

      Options options = new Options();
      options.addOption("h", "help", false, "This is a looooong description");
      
      HelpFormatter formatter = new HelpFormatter();
      formatter.setWidth(20);
      formatter.printHelp("app", options); // hang & crash
      

      An helpful exception indicating the insufficient width would be more appropriate than an OutOfMemoryError.

      1. CLI-162-additional.patch
        2 kB
        Henri Yandell
      2. CLI-162-take2.patch
        2 kB
        Henri Yandell

        Issue Links

          Activity

          Hide
          Henri Yandell added a comment -

          Throw IllegalStateException on last infinite loop test case.

          svn ci -m "Applying additional patch to throw IllegalStateException when the speci
          fied width is not enough to fit the flags, indent and 1 character for the description. This closes out CLI-162 (for now ). "
          Sending src/java/org/apache/commons/cli/HelpFormatter.java
          Sending src/test/org/apache/commons/cli/bug/BugCLI162Test.java
          Transmitting file data ..
          Committed revision 746137.

          Show
          Henri Yandell added a comment - Throw IllegalStateException on last infinite loop test case. svn ci -m "Applying additional patch to throw IllegalStateException when the speci fied width is not enough to fit the flags, indent and 1 character for the description. This closes out CLI-162 (for now ). " Sending src/java/org/apache/commons/cli/HelpFormatter.java Sending src/test/org/apache/commons/cli/bug/BugCLI162Test.java Transmitting file data .. Committed revision 746137.
          Hide
          Henri Yandell added a comment -

          Ended up modifying this to break it up to fit screen width.

          Discovered another infinite loop issue if the entered width is smaller than the nextTabStop (ie: argument + indent).

          Show
          Henri Yandell added a comment - Ended up modifying this to break it up to fit screen width. Discovered another infinite loop issue if the entered width is smaller than the nextTabStop (ie: argument + indent).
          Hide
          Henri Yandell added a comment -

          svn ci -m "Applying my second attempt at a patch to CLI-162. This fixes Gary's reported bug (one of which was an example of CLI-162, and one a bug in my first attempt to patch). Open question is whether to output text that is too long, or try and break it up to fit the screen width. "

          Sending src/java/org/apache/commons/cli/HelpFormatter.java
          Sending src/test/org/apache/commons/cli/bug/BugCLI162Test.java
          Transmitting file data ..
          Committed revision 745388.

          Show
          Henri Yandell added a comment - svn ci -m "Applying my second attempt at a patch to CLI-162 . This fixes Gary's reported bug (one of which was an example of CLI-162 , and one a bug in my first attempt to patch). Open question is whether to output text that is too long, or try and break it up to fit the screen width. " Sending src/java/org/apache/commons/cli/HelpFormatter.java Sending src/test/org/apache/commons/cli/bug/BugCLI162Test.java Transmitting file data .. Committed revision 745388.
          Hide
          Henri Yandell added a comment -

          Attaching patch that rolls back the previous RuntimeException throwing. The if statement in that patch was testing the wrong condition. This patch adds the correct condition, and rather than throwing an exception the text is simply outputted irregardless of the fact it is over the width. What should happen is debatable here - due to a side-effect of CLI-151, we can't do anything aggressive here because things were printing out happily if they were under width + printTabStop. Our options are either to just print, or to forcibly break the text.

          The test code no longer expects to get a RuntimeException.

          Show
          Henri Yandell added a comment - Attaching patch that rolls back the previous RuntimeException throwing. The if statement in that patch was testing the wrong condition. This patch adds the correct condition, and rather than throwing an exception the text is simply outputted irregardless of the fact it is over the width. What should happen is debatable here - due to a side-effect of CLI-151 , we can't do anything aggressive here because things were printing out happily if they were under width + printTabStop. Our options are either to just print, or to forcibly break the text. The test code no longer expects to get a RuntimeException.
          Hide
          Henri Yandell added a comment -

          Two of the options appear to be problematic in CLI162.

          The first is OPT_PASSWORD. In this the url is longer than the allowed width of the screen, so some kind of failure needs to happen - or the url needs to be mercilessly chopped. This is the one that goes into an infinite loop due to CLI-151. My gut feeling from the code is that CLI-151 didn't introduce this bug, but you had to be significantly longer to hit the bug before (ie: printTabStop longer than the width).

          The second is OPT_PARAM_TYPES_INT + OPT_PARAM_TYPES_NAME, it shows the the patch for this ticket contained a bug when the lastPos happened to equal the firstPos for completely normal reasons. I'd missed that pos was already set to a real value when the loop was begun and not to 0. I'm not sure why both options have to be set for this to happen.

          Show
          Henri Yandell added a comment - Two of the options appear to be problematic in CLI162. The first is OPT_PASSWORD. In this the url is longer than the allowed width of the screen, so some kind of failure needs to happen - or the url needs to be mercilessly chopped. This is the one that goes into an infinite loop due to CLI-151 . My gut feeling from the code is that CLI-151 didn't introduce this bug, but you had to be significantly longer to hit the bug before (ie: printTabStop longer than the width). The second is OPT_PARAM_TYPES_INT + OPT_PARAM_TYPES_NAME, it shows the the patch for this ticket contained a bug when the lastPos happened to equal the firstPos for completely normal reasons. I'd missed that pos was already set to a real value when the loop was begun and not to 0. I'm not sure why both options have to be set for this to happen.
          Hide
          ggregory@seagullsw.com added a comment -

          Committed new test in BugCLI162Test. The class BugCLI162Test is not invoked during a Maven build BTW.

          Show
          ggregory@seagullsw.com added a comment - Committed new test in BugCLI162Test. The class BugCLI162Test is not invoked during a Maven build BTW.
          Hide
          Henri Yandell added a comment -

          svn ci -m "Changing the current OutOfMemoryError to a RuntimeException per CLI-162. A new ticket for the RuntimeException is at CLI-174"

          Sending src/java/org/apache/commons/cli/HelpFormatter.java
          Adding src/test/org/apache/commons/cli/bug/BugCLI162Test.java
          Transmitting file data ..
          Committed revision 735257.

          Show
          Henri Yandell added a comment - svn ci -m "Changing the current OutOfMemoryError to a RuntimeException per CLI-162 . A new ticket for the RuntimeException is at CLI-174 " Sending src/java/org/apache/commons/cli/HelpFormatter.java Adding src/test/org/apache/commons/cli/bug/BugCLI162Test.java Transmitting file data .. Committed revision 735257.

            People

            • Assignee:
              Unassigned
              Reporter:
              Emmanuel Bourg
            • Votes:
              0 Vote for this issue
              Watchers:
              0 Start watching this issue

              Dates

              • Created:
                Updated:
                Resolved:

                Development