DO NOT REPLY [Bug 16204] New: - Infinite loop in StringUtils.replace(text, repl, with) + FIX

[bugzilla]

DO NOT REPLY TO THIS EMAIL, BUT PLEASE POST YOUR BUG 
RELATED COMMENTS THROUGH THE WEB INTERFACE AVAILABLE AT
<http://nagoya.apache.org/bugzilla/show_bug.cgi?id=16204>.
ANY REPLY MADE TO THIS MESSAGE WILL NOT BE COLLECTED AND 
INSERTED IN THE BUG DATABASE.

http://nagoya.apache.org/bugzilla/show_bug.cgi?id=16204

Infinite loop in StringUtils.replace(text, repl, with) + FIX

           Summary: Infinite loop in StringUtils.replace(text, repl, with) +
                    FIX
           Product: Commons
           Version: 1.0 Alpha
          Platform: PC
        OS/Version: Linux
            Status: NEW
          Severity: Normal
          Priority: Other
         Component: Lang
        AssignedTo: [EMAIL PROTECTED]
        ReportedBy: [EMAIL PROTECTED]


Hopefully I can give back a little after having had so much
benefit of your work.

I will refer to the sources from commons-lang-1.0.1-src.zip, downloaded
the 17 January 2003.

Detail:
=======
In org.apache.commons.lang.StringUtils: If you invoke
public static String replace(String text, String repl, String with)
with arguments:
   text  !=  null
   repl.equals("")
   with  anything

you get an infinite loop as "FOO".indexOf("") == 0.

Demo:
=====
To demonstrate the bug, please add the following lines in
org.apache.commons.lang.StringUtilsTest in the body of
testReplaceFunctions(), line 194:

    public void testReplaceFunctions() {

      //... existing code

      //-- bug demonstration, added by HoKr
      assertEquals("replace(String, String, String) failed",
          "FOO", StringUtils.replace("FOO", "", "any"));
    }

I got an OutOfMemoryException then.

Fix:
====
My suggestion to fix this in StringUtils.replace(String, String, String),
line 593:

    public static String replace(String text, String repl, String with,
                                 int max) {
        if (text == null) {
            return null;
        }

        //-- FIX SUGGESTION START >>>
        //-- added by HoKr for infinite loop avoidance
        //-- keeps on throwing NullPointerException if repl == null
        //-- -->> this is faster than "".equals(repl); NPE allowed.
        if (repl.length() == 0) {
           return text;
        }
        //-- <<< FIX SUGGESTION END

        StringBuffer buf = new StringBuffer(text.length());
        int start = 0, end = 0;
        while ((end = text.indexOf(repl, start)) != -1) {
            buf.append(text.substring(start, end)).append(with);
            start = end + repl.length();

            if (--max == 0) {
                break;
            }
        }
        buf.append(text.substring(start));
        return buf.toString();
    }

Further:
========
Further I suggest instead of throwing NullPointerExceptions
if (repl == null || with == null) to return the parameter text then.

It would meet closer the expectation of what the method should perform from
my point of view in these cases.

This behaviour would be payed with 2 extra comparisons to null
(before the while-loop) in 'normal' operation mode though.

The Code would be:

    public static String replace(String text, String repl, String with,
                                 int max) {
        if (text == null) {
            return null;
        }

        //-- START >>>
        //-- suggestion by HoKr, BUT would CHANGE outside behaviour:
        //-- not throwing NPE any more!
        if (repl == null || with == null) {
            return text;
        }
        //-- added by HoKr for infinite loop avoidance
        //-- keeps on throwing NullPointerException if repl == null
        if (repl.length() == 0) {
            return text;
        }
        //-- <<< END

        StringBuffer buf = new StringBuffer(text.length());
        int start = 0, end = 0;
        while ((end = text.indexOf(repl, start)) != -1) {
            buf.append(text.substring(start, end)).append(with);
            start = end + repl.length();

            if (--max == 0) {
                break;
            }
        }
        buf.append(text.substring(start));
        return buf.toString();
    }



Regards,
Holger Krauth

--
To unsubscribe, e-mail:   <mailto:[EMAIL PROTECTED]>
For additional commands, e-mail: <mailto:[EMAIL PROTECTED]>