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@xxxxxxx>
>>> ---
>>> 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@xxxxxxxxxx>
> --Mark.
>
> _______________________________________________
> xfs mailing list
> xfs@xxxxxxxxxxx
> http://oss.sgi.com/mailman/listinfo/xfs
>
_______________________________________________
xfs mailing list
xfs@xxxxxxxxxxx
http://oss.sgi.com/mailman/listinfo/xfs
