Uploaded image for project: 'Commons CLI'
  1. Commons CLI
  2. CLI-193

StringIndexOutOfBoundsException in HelpFormatter.findWrapPos

    Details

    • Type: Bug
    • Status: Closed
    • Priority: Critical
    • Resolution: Fixed
    • Affects Version/s: 1.2
    • Fix Version/s: 1.3
    • Component/s: CLI-1.x
    • Labels:
      None

      Description

      In the last while loop in HelpFormatter.findWrapPos, it can pass text.length() to text.charAt(int), which throws a StringIndexOutOfBoundsException. The first expression in that while loop condition should use a <, not a <=.

      This is on line 908 in r779646:
      http://svn.apache.org/viewvc/commons/proper/cli/trunk/src/java/org/apache/commons/cli/HelpFormatter.java?revision=779646&view=markup

        Activity

        Hide
        ebourg Emmanuel Bourg added a comment -

        Thank you for the report Travis. Do you have a test case that triggers the exception?

        Show
        ebourg Emmanuel Bourg added a comment - Thank you for the report Travis. Do you have a test case that triggers the exception?
        Hide
        chris.lee Chris Lee added a comment -

        Here is a patch that adds a test case and applies the fix suggested by Travis.

        Show
        chris.lee Chris Lee added a comment - Here is a patch that adds a test case and applies the fix suggested by Travis.
        Hide
        motravo Travis McLeskey added a comment -

        You should be able to get it to throw with a call like this:

        findWrapPos( "hello", 3, 0 ); // should return -1

        The exception gets thrown whenever the passed-in string does not contain any spaces, tabs, or newlines. It looks like line 914 was also expecting line 908 to have a <.

        Also, I think there are other bugs. In this call:

        findWrapPos( "helloooo\noo\ngoodbye", 6, 7 ); // should return 8

        we want it to short circuit and return the first newline if there is one in the substring starting at 7 with width 6 ("o\noo\ng"), and so it should return the index of the first '\n', which is 8. However, on lines 876 and 877, it's doing "pos <= width", instead of "pos <= startPos+width", so it won't return, and it will end up (incorrectly, I think) returning the index of the second newline's position. And once you fix lines 876 and 877, it's going to return 9 instead of 8, which I think is incorrect. The other return statements return pos, not pos+1, and I think line 879 should do the same.

        Show
        motravo Travis McLeskey added a comment - You should be able to get it to throw with a call like this: findWrapPos( "hello", 3, 0 ); // should return -1 The exception gets thrown whenever the passed-in string does not contain any spaces, tabs, or newlines. It looks like line 914 was also expecting line 908 to have a <. Also, I think there are other bugs. In this call: findWrapPos( "helloooo\noo\ngoodbye", 6, 7 ); // should return 8 we want it to short circuit and return the first newline if there is one in the substring starting at 7 with width 6 ("o\noo\ng"), and so it should return the index of the first '\n', which is 8. However, on lines 876 and 877, it's doing "pos <= width", instead of "pos <= startPos+width", so it won't return, and it will end up (incorrectly, I think) returning the index of the second newline's position. And once you fix lines 876 and 877, it's going to return 9 instead of 8, which I think is incorrect. The other return statements return pos, not pos+1, and I think line 879 should do the same.
        Hide
        ebourg Emmanuel Bourg added a comment -

        The proposed change makes sense but it doesn't solve completely the issue with "unbreakable" text.

        For example, considering the following test:

        public void testRenderWrappedTextWordCut()
        {
            int width = 7;
            int padding = 0;
            String text = "Thisisatest.";
            String expected = "Thisisa" + EOL + 
                              "test.";
            
            StringBuffer sb = new StringBuffer();
            new HelpFormatter().renderWrappedText(sb, width, padding, text);
            System.out.println(sb.toString());
            assertEquals("cut and wrap", expected, sb.toString());
        }
        

        With the current code it throws a StringIndexOutOfBoundsException. With the modification it fails because the text is not cut.

        Show
        ebourg Emmanuel Bourg added a comment - The proposed change makes sense but it doesn't solve completely the issue with "unbreakable" text. For example, considering the following test: public void testRenderWrappedTextWordCut() { int width = 7; int padding = 0; String text = "Thisisatest." ; String expected = "Thisisa" + EOL + "test." ; StringBuffer sb = new StringBuffer (); new HelpFormatter().renderWrappedText(sb, width, padding, text); System .out.println(sb.toString()); assertEquals( "cut and wrap" , expected, sb.toString()); } With the current code it throws a StringIndexOutOfBoundsException. With the modification it fails because the text is not cut.
        Hide
        chris.lee Chris Lee added a comment -

        Well, that would be easy enough to fix, but that's at odds with how the method is documented now. It currently says it will ONLY break on a newline or space ("The wrap point is the last postion before startPos+width having a whitespace character (space, \n, \r).").

        In addition to that, it's currently looking for whitespace AFTER startPos+width (also at odds with documentation). How should it actually behave?

        I've attached another patch that will preserve the existing functionality (looking for spaces after startPos+width) but reverts back to returning startPos+width if none is found.

        If, instead, it should never allow for a string longer than startPos+width, then current lines 908-912 should probably just be removed. This will cause "wrap position 3" unit test to fail though.

        Show
        chris.lee Chris Lee added a comment - Well, that would be easy enough to fix, but that's at odds with how the method is documented now. It currently says it will ONLY break on a newline or space ("The wrap point is the last postion before startPos+width having a whitespace character (space, \n, \r)."). In addition to that, it's currently looking for whitespace AFTER startPos+width (also at odds with documentation). How should it actually behave? I've attached another patch that will preserve the existing functionality (looking for spaces after startPos+width) but reverts back to returning startPos+width if none is found. If, instead, it should never allow for a string longer than startPos+width, then current lines 908-912 should probably just be removed. This will cause "wrap position 3" unit test to fail though.
        Hide
        ebourg Emmanuel Bourg added a comment -

        I believe the absolute rule for the wrapping should be to not exceed the maximum width, then to do the best effort to wrap on space characters, and if it is not possible, break a word.

        Show
        ebourg Emmanuel Bourg added a comment - I believe the absolute rule for the wrapping should be to not exceed the maximum width, then to do the best effort to wrap on space characters, and if it is not possible, break a word.
        Hide
        chris.lee Chris Lee added a comment -

        Makes sense to me too. Doesn't help to have characters run only a little bit off the screen

        Here is an updated patch that will never exceed startPos+width.

        Show
        chris.lee Chris Lee added a comment - Makes sense to me too. Doesn't help to have characters run only a little bit off the screen Here is an updated patch that will never exceed startPos+width.
        Hide
        ebourg Emmanuel Bourg added a comment -

        Thank you for the patch Chris. I applied the change you suggested and it seems to work fine.

        Show
        ebourg Emmanuel Bourg added a comment - Thank you for the patch Chris. I applied the change you suggested and it seems to work fine.

          People

          • Assignee:
            Unassigned
            Reporter:
            motravo Travis McLeskey
          • Votes:
            0 Vote for this issue
            Watchers:
            2 Start watching this issue

            Dates

            • Created:
              Updated:
              Resolved:

              Time Tracking

              Estimated:
              Original Estimate - 5m
              5m
              Remaining:
              Remaining Estimate - 5m
              5m
              Logged:
              Time Spent - Not Specified
              Not Specified

                Development