SEGV on XrdOucString replace(...) method

[faluchet]

xrootd/src/XrdOuc/XrdOucString.cc

Line 733 in de93530

int XrdOucString::replace(const char *s1, const char *s2, int from, int to)

replace() method on some strings triggers "SEGV signal caused by a WRITE memory access".

For example:
XrdOucString good_s {"aaa"};
good_s.replace("a","xa"} // no problem

// But
XrdOucString bad_s {"aaaa"};
bad_s.replace("a","xa"} // SEGV on write

// Anyway
XrdOucString another_good_s {"abaa"};
another_good_s.replace("a","xa"} // no problem

Trying with a few other sample strings, it looks like there is a pattern with "faulty strings" being longer than 4 chars and with 2 equal starting characters, when the string to be replaced is a substring of the replacing string.
( Just an hint, it has not been rigorously proven though ).

[simonmichal]

Adding valgrind output:

==17216== Invalid write of size 1
==17216==    at 0x4C2EA73: memcpy@@GLIBC_2.14 (vg_replace_strmem.c:1035)
==17216==    by 0x5C1EF5F: XrdOucString::replace(char const*, char const*, int, int) (XrdOucString.cc:808)
==17216==    by 0x400B0D: main (xrd_api_test.cc:13)
==17216==  Address 0xa4b57df is 1 bytes before a block of size 9 alloc'd
==17216==    at 0x4C2C1C1: realloc (vg_replace_malloc.c:836)
==17216==    by 0x5C1DFE8: XrdOucString::bufalloc(int) (XrdOucString.cc:121)
==17216==    by 0x5C1F03A: XrdOucString::replace(char const*, char const*, int, int) (XrdOucString.cc:770)
==17216==    by 0x400B0D: main (xrd_api_test.cc:13)
==17216== 
==17216== Invalid write of size 1
==17216==    at 0x4C2E798: memcpy@@GLIBC_2.14 (vg_replace_strmem.c:1035)
==17216==    by 0x5C1EF5F: XrdOucString::replace(char const*, char const*, int, int) (XrdOucString.cc:808)
==17216==    by 0x400B0D: main (xrd_api_test.cc:13)
==17216==  Address 0xa4b57dd is 3 bytes before a block of size 9 alloc'd
==17216==    at 0x4C2C1C1: realloc (vg_replace_malloc.c:836)
==17216==    by 0x5C1DFE8: XrdOucString::bufalloc(int) (XrdOucString.cc:121)
==17216==    by 0x5C1F03A: XrdOucString::replace(char const*, char const*, int, int) (XrdOucString.cc:770)
==17216==    by 0x400B0D: main (xrd_api_test.cc:13)
==17216== 
==17216== Invalid write of size 1
==17216==    at 0x4C2E693: memcpy@GLIBC_2.2.5 (vg_replace_strmem.c:1034)
==17216==    by 0x5C1EF42: XrdOucString::replace(char const*, char const*, int, int) (XrdOucString.cc:806)
==17216==    by 0x400B0D: main (xrd_api_test.cc:13)
==17216==  Address 0xa4b57dd is 3 bytes before a block of size 9 alloc'd
==17216==    at 0x4C2C1C1: realloc (vg_replace_malloc.c:836)
==17216==    by 0x5C1DFE8: XrdOucString::bufalloc(int) (XrdOucString.cc:121)
==17216==    by 0x5C1F03A: XrdOucString::replace(char const*, char const*, int, int) (XrdOucString.cc:770)
==17216==    by 0x400B0D: main (xrd_api_test.cc:13)
==17216== 
==17216== Invalid write of size 1
==17216==    at 0x4C2E58C: memcpy@GLIBC_2.2.5 (vg_replace_strmem.c:1034)
==17216==    by 0x5C1EF42: XrdOucString::replace(char const*, char const*, int, int) (XrdOucString.cc:806)
==17216==    by 0x400B0D: main (xrd_api_test.cc:13)
==17216==  Address 0xa4b57d9 is 7 bytes before a block of size 9 alloc'd
==17216==    at 0x4C2C1C1: realloc (vg_replace_malloc.c:836)
==17216==    by 0x5C1DFE8: XrdOucString::bufalloc(int) (XrdOucString.cc:121)
==17216==    by 0x5C1F03A: XrdOucString::replace(char const*, char const*, int, int) (XrdOucString.cc:770)
==17216==    by 0x400B0D: main (xrd_api_test.cc:13)
==17216== 
==17216== 
==17216== Process terminating with default action of signal 11 (SIGSEGV)
==17216==  Bad permissions for mapped region at address 0xA466FFF
==17216==    at 0x4C2EA73: memcpy@@GLIBC_2.14 (vg_replace_strmem.c:1035)
==17216==    by 0x5C1EF5F: XrdOucString::replace(char const*, char const*, int, int) (XrdOucString.cc:808)
==17216==    by 0x400B0D: main (xrd_api_test.cc:13)
--17216-- VALGRIND INTERNAL ERROR: Valgrind received a signal 11 (SIGSEGV) - exiting
--17216-- si_code=128;  Faulting address: 0x0;  sp: 0x1002ba9c10

[simonmichal]

In case of string like "aaaa" and replacement "a" -> "xa" in following code snippet:

xrootd/src/XrdOuc/XrdOucString.cc

Lines 802 to 811 in 367f86f

	while (at > -1 && at >= from) {
	int ln = atn - at - l1;
	char *pc = str + at + l1 + nc*dd;
	if (ln > 0)
	memmove(pc,str+at+l1,ln);
	if (l2 > 0)
	memcpy(pc-l2,s2,l2);
	nc--;
	atn = at;
	at = rfind(s1,at-l1);

after 4 iterations the algorithm replaces "aaaa" with "xaxaxaxa" and at = 0 and l1 = 1 (length of the to be replaced string) at that point we call:

xrootd/src/XrdOuc/XrdOucString.cc

Line 811 in 367f86f

at = rfind(s1,at-l1);

substituting the variables with values: at = rfind("a",-1);, in the context of rfind the -1 constant has a special meaning (STR_NPOS) -> search starts from end of string, hence although we are basically done the algorithm starts again scanning the whole string and as a result at some point the output buffer gets miscalculated at some point which results with a segv.