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

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

[Mark Tinguely]

[-- Attachment #1: xfsrestore-fix-string-corruption-in-shrink.patch --]
[-- Type: text/plain, Size: 1402 bytes --]

Linux strcpy() corrupts the output string when the input
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
+	 * 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';
 }
 
 static int


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

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

[Eric Sandeen]

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


>  static int
> 
> 
> _______________________________________________
> 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

Re: [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@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.

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

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

[Eric Sandeen]

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

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

[Mark Tinguely]

On 11/13/14 14:45, Eric Sandeen wrote:
> 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(-)

>> 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>

No offense to Linux intended.

liter is an array of mostly NULL characters that calls this function. 
The contents of the array seems at odds with how this function operates. 
The comment was mostly for my future self so I don't have to relearn 
what is going on here. I do not mind it removed from the community code.

--Mark.

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

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

[Dave Chinner]

On Thu, Nov 13, 2014 at 01:56:17PM -0600, 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.

I'd much prefer the strlen() + memmove() because then it's self
documenting that the move of the string is overlapping. It will also
be faster for large strings than a manual byte-at-a-time copy,
and there's less code to maintain overall. ;)

Cheers,

Dave.
-- 
Dave Chinner
david@fromorbit.com

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