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

[Eric Sandeen <sandeen@sandeen.net>]

On 11/13/14 1:56 PM, Mark Tinguely wrote:
> 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@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.

Ok, fair enough.  I think mine is clearer to dummies like me, but *shrug.*

I'd also prefer that the comment say "overlapping strcpy is undefined"
rather than poking fun at Linux. ;)  And the rest of the comment doesn't
really help me understand what's going on.  "liter?"  "embedded processing
characters?"  I have no idea what that means when I'm reading this function,
which simply moves part of a string around, so I'd rather have either a
description of what it does & doesn't do at the
top, or leave out those seemingly random details.

But it fixes a bug, and those are nitpicks you can fix, or not, I suppose,
though I really would prefer a clearer comment so future readers have some
clue, and don't end up more confused than when they started reading it. ;)

Anyway, for the fix itself,

Reviewed-by: Eric Sandeen <sandeen@redhat.com>

> --Mark.
> 
> _______________________________________________
> xfs mailing list
> xfs@oss.sgi.com
> http://oss.sgi.com/mailman/listinfo/xfs
> 

_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs