[LANG-15] Infinite loop in StringUtils.replace(text, repl, with) + FIX

• Type: Bug
• Status: Closed
• Priority: Major
• Resolution: Fixed
• Affects Version/s: 2.0
• Fix Version/s: 2.0
• Component/s: lang.*

Environment

Operating System: Linux
Platform: PC

• Bugzilla Id: 16204
• Assignee: Unassigned
• Reporter: Holger Krauth
• Created: 17/Jan/03 22:44
• Updated: 16/Dec/09 08:50
• Resolved: 16/Dec/09 08:50

Description

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