xfs
[Top] [All Lists]

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

To: Mark Tinguely <tinguely@xxxxxxx>
Subject: Re: [PATCH] xfsrestore: fix string corruption in shrink()
From: Dave Chinner <david@xxxxxxxxxxxxx>
Date: Fri, 14 Nov 2014 08:50:36 +1100
Cc: Eric Sandeen <sandeen@xxxxxxxxxxx>, xfs@xxxxxxxxxxx
Delivered-to: xfs@xxxxxxxxxxx
In-reply-to: <54650CE1.3050706@xxxxxxx>
References: <20141113191510.176392492@xxxxxxx> <54650970.7010902@xxxxxxxxxxx> <54650CE1.3050706@xxxxxxx>
User-agent: Mutt/1.5.21 (2010-09-15)
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@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.

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

<Prev in Thread] Current Thread [Next in Thread>