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-take2.patch
        2 kB
        Henri Yandell
      2. CLI-162-additional.patch
        2 kB
        Henri Yandell

        Issue Links

          Activity

          Transition Time In Source Status Execution Times Last Executer Last Execution Date
          Open Open Closed Closed
          175d 21h 9m 1 Henri Yandell 17/Jan/09 09:31
          Closed Closed Reopened Reopened
          23d 19h 16m 1 ggregory@seagullsw.com 10/Feb/09 04:47
          Reopened Reopened Closed Closed
          10d 13m 1 Henri Yandell 20/Feb/09 05:01
          Mark Thomas made changes -
          Workflow jira [ 12436041 ] Default workflow, editable Closed status [ 12601477 ]
          Henri Yandell made changes -
          Status Reopened [ 4 ] Closed [ 6 ]
          Resolution Fixed [ 1 ]
          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.
          Henri Yandell made changes -
          Attachment CLI-162-additional.patch [ 12400574 ]
          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.
          Henri Yandell made changes -
          Attachment CLI-162-take2.patch [ 12400385 ]
          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.
          ggregory@seagullsw.com made changes -
          Status Closed [ 6 ] Reopened [ 4 ]
          Resolution Fixed [ 1 ]
          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.
          Henri Yandell made changes -
          Status Open [ 1 ] Closed [ 6 ]
          Resolution Fixed [ 1 ]
          Fix Version/s 1.2 [ 12312674 ]
          Fix Version/s 1.3 [ 12313321 ]
          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.
          Henri Yandell made changes -
          Field Original Value New Value
          Link This issue is cloned as CLI-174 [ CLI-174 ]
          Emmanuel Bourg created issue -

            People

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

              Dates

              • Created:
                Updated:
                Resolved:

                Development