xfs
[Top] [All Lists]

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

To: Eric Sandeen <sandeen@xxxxxxxxxxx>
Subject: Re: [PATCH] xfsrestore: fix string corruption in shrink()
From: Mark Tinguely <tinguely@xxxxxxx>
Date: Thu, 13 Nov 2014 15:57:29 -0600
Cc: xfs@xxxxxxxxxxx
Delivered-to: xfs@xxxxxxxxxxx
In-reply-to: <5465187D.3060308@xxxxxxxxxxx>
References: <20141113191510.176392492@xxxxxxx> <54650970.7010902@xxxxxxxxxxx> <54650CE1.3050706@xxxxxxx> <5465187D.3060308@xxxxxxxxxxx>
User-agent: Mozilla/5.0 (X11; FreeBSD amd64; rv:9.0) Gecko/20120122 Thunderbird/9.0
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@xxxxxxx>
---
   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@xxxxxxxxxx>

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.

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