xfsrestore: fix string corruption in shrink()
PATCH 1/1 xfsrestore: fix string corruption in shrink()
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.
--- restore/tree.c | 14 +++++++++++++- 1 file changed, 13 insertions(+), 1 deletion(-)
restore/tree.c +13 -1
@@ -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
[email protected]
http://oss.sgi.com/mailman/listinfo/xfs
Quoted 1 line · Mark Tinguely
Linux
strcpy()corrupts the output string when the input
Not Linux strcpy in particular; per C99:
Quoted 4 lines
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.
^^^^^^^^^^^^^^^^^^^^^^^^^
Quoted 23 lines
and output strings overlap. The
shrink()function in xfsrestore uses an overlappingstrcpy()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 foundFix my manually moving the characters in the array.
Signed-off-by Mark Tinguely <[email protected]>--- restore/tree.c | 14 +++++++++++++- 1 file changed, 13 insertions(+), 1 deletion(-)Index:
b/restore/tree.c
===================================================================
restore/tree.c distance_to_space( char *s, char *l ) · line 4864
Quoted 9 lines
restore/tree.c +5
-1@@ -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. ;)
restore/tree.c distance_to_space( char *s, char *l ) · line 4873
Quoted 9 lines
restore/tree.c +8
-0+* 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
Quoted 8 lines
restore/tree.c +0
-0static int_______________________________________________
xfs mailing list
[email protected]
http://oss.sgi.com/mailman/listinfo/xfs
_______________________________________________
xfs mailing list
[email protected]
http://oss.sgi.com/mailman/listinfo/xfs
Quoted 58 lines · Eric Sandeen
Quoted 1 line · Mark Tinguely
Linux
strcpy()corrupts the output string when the inputNot Linux strcpy in particular; per C99:
Quoted 4 lines
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.
^^^^^^^^^^^^^^^^^^^^^^^^^Quoted 23 lines
and output strings overlap. The
shrink()function in xfsrestore uses an overlappingstrcpy()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 foundFix my manually moving the characters in the array.
Signed-off-by Mark Tinguely<[email protected]>--- restore/tree.c | 14 +++++++++++++- 1 file changed, 13 insertions(+), 1 deletion(-)Index:
b/restore/tree.c
===================================================================restore/tree.c distance_to_space( char *s, char *l ) · line 4864
Quoted 9 lines
restore/tree.c +5
-1@@ -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. ;)
restore/tree.c distance_to_space( char *s, char *l ) · line 4873
Quoted 9 lines
restore/tree.c +8
-0+* 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?
Quoted 8 lines
size_tn = 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
[email protected]
http://oss.sgi.com/mailman/listinfo/xfs
Eric Sandeen
Reviewed-byQuoted 61 lines · Mark Tinguely
Quoted 60 lines · Eric Sandeen
Quoted 1 line · Mark Tinguely
Linux
strcpy()corrupts the output string when the inputNot Linux strcpy in particular; per C99:
Quoted 4 lines
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.
^^^^^^^^^^^^^^^^^^^^^^^^^Quoted 23 lines
and output strings overlap. The
shrink()function in xfsrestore uses an overlappingstrcpy()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 foundFix my manually moving the characters in the array.
Signed-off-by Mark Tinguely<[email protected]>--- restore/tree.c | 14 +++++++++++++- 1 file changed, 13 insertions(+), 1 deletion(-)Index:
b/restore/tree.c
===================================================================restore/tree.c distance_to_space( char *s, char *l ) · line 4864
Quoted 9 lines
restore/tree.c +5
-1@@ -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. ;)
restore/tree.c distance_to_space( char *s, char *l ) · line 4873
Quoted 9 lines
restore/tree.c +8
-0+* 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_tn = strlen(s+cnt) + 1; /* 1 for terminating NULL */
Quoted 8 lines
Quoted 6 lines
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,
Quoted 7 lines
--Mark.
_______________________________________________
xfs mailing list
[email protected]
http://oss.sgi.com/mailman/listinfo/xfs
_______________________________________________
xfs mailing list
[email protected]
http://oss.sgi.com/mailman/listinfo/xfs
Quoted 43 lines · Eric Sandeen
Quoted 23 lines · Mark Tinguely
Quoted 22 lines · Eric Sandeen
Quoted 1 line · Mark Tinguely
Linux
strcpy()corrupts the output string when the inputNot Linux strcpy in particular; per C99:
Quoted 4 lines
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.
^^^^^^^^^^^^^^^^^^^^^^^^^Quoted 11 lines
and output strings overlap. The
shrink()function in xfsrestore uses an overlappingstrcpy()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 foundQuoted 8 lines
Quoted 6 lines
Quoted 6 lines
Fix my manually moving the characters in the array.
Signed-off-by Mark Tinguely<[email protected]>--- 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.
Quoted 7 lines
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<[email protected]>
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
[email protected]
http://oss.sgi.com/mailman/listinfo/xfs
Quoted 63 lines · Mark Tinguely
Quoted 62 lines · Eric Sandeen
Quoted 1 line · Mark Tinguely
Linux
strcpy()corrupts the output string when the inputNot Linux strcpy in particular; per C99:
Quoted 4 lines
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.
^^^^^^^^^^^^^^^^^^^^^^^^^Quoted 23 lines
and output strings overlap. The
shrink()function in xfsrestore uses an overlappingstrcpy()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 foundFix my manually moving the characters in the array.
Signed-off-by Mark Tinguely<[email protected]>--- restore/tree.c | 14 +++++++++++++- 1 file changed, 13 insertions(+), 1 deletion(-)Index:
b/restore/tree.c
===================================================================restore/tree.c distance_to_space( char *s, char *l ) · line 4864
Quoted 9 lines
restore/tree.c +5
-1@@ -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. ;)
restore/tree.c distance_to_space( char *s, char *l ) · line 4873
Quoted 9 lines
restore/tree.c +8
-0+* 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_tn = strlen(s+cnt) + 1; /* 1 for terminating NULL */
memmove(s, s + cnt, n);
Quoted 7 lines
Quoted 4 lines
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.
Signature · 7 lines
Dave Chinner [email protected] _______________________________________________ xfs mailing list [email protected] http://oss.sgi.com/mailman/listinfo/xfs