[PATCH] xfsrestore: fix string corruption in shrink()

[Mark Tinguely]

On 11/13/14 13:41, Eric Sandeen wrote:
> On 11/13/14 1:14 PM, Mark Tinguely wrote:
>> Linux strcpy() corrupts the output string when the input
>
> Not Linux strcpy in particular; per C99:
>
>> The strcpy function copies the string pointed to by s2
>> (including the terminating null character) into the array
>> pointed to by s1. If copying takes place between objects
>> that overlap, the behavior is undefined.
>                  ^^^^^^^^^^^^^^^^^^^^^^^^^
>
>> and output strings overlap. The shrink() function in xfsrestore
>> uses an overlapping strcpy() to remove special characters when
>> processing an interactive command line. The resultant command
>> will fail.
>>
>> examples:
>>   ->  cd "AOGC exome chip core genotyping"
>> AOGC exome chp  core genotyping not found
>>   ->  cd "t t"
>> tt not found
>>
>> Fix my manually moving the characters in the array.
>>
>> Signed-off-by: Mark Tinguely<tinguely at sgi.com>
>> ---
>>   restore/tree.c |   14 +++++++++++++-	
>>   1 file changed, 13 insertions(+), 1 deletion(-)
>>
>> Index: b/restore/tree.c
>> ===================================================================
>> --- a/restore/tree.c
>> +++ b/restore/tree.c
>> @@ -4857,7 +4857,19 @@ distance_to_space( char *s, char *l )
>>   static void
>>   shrink( char *s, size_t cnt )
>>   {
>> -	strcpy( s, s + cnt );
>> +	/*
>> +	 * Linux strcpy corrupts the string if the src and dst overlap.
>> +	 * Manually copy the entries to the left.
>> +	 *
>> +	 * Since the liter array is mostly nulls, shrink is not moving
>
> what is the "liter array?"  Ah well.  Context.  ;)
>
>> +	 * the array left as intended. Does not seem to be many embedded
>> +	 * processing characters, so leaving it for now
>> +	 */
>> +	char *m = s + cnt;
>> +	while (*m != '\0')
>> +		*s++ = *m++;
>> +	/* NULL the last character of the string */
>> +	*s = '\0';
>>   }
>
> Would this be any less manual?
>
>      size_t n = strlen(s+cnt) + 1; /* 1 for terminating NULL */
>
>      memmove(s, s + cnt, n);
>
> because memmove is ok with overlaps.
>
> -Eric
>

I thought of that but if we are doing a strlen() might as well just copy 
it while you are walking the string.

--Mark.