[PATCH] xfsdump: fill in missing strerror() call

[Nigel Tamplin]

On 26/12/12 22:25, Alex Elder wrote:
> Nigel Tamplin reported getting a seg fault in xfsrestore when a path
> name was too long.
>
> Based on the surrounding code, I'm sure strerror(errno) was the
> intended final argument to this call.  This bug has been there
> since the code was first committed.
>
> Signed-off-by: Alex Elder<elder at inktank.com>
> Reported-by: Nigel Tamplin<ntamplin at codefaber.co.uk>
> ---
>   restore/content.c |    3 ++-
>   1 file changed, 2 insertions(+), 1 deletion(-)
>
> diff --git a/restore/content.c b/restore/content.c
> index edd00ed..4e55a76 100644
> --- a/restore/content.c
> +++ b/restore/content.c
> @@ -7799,7 +7799,8 @@ restore_spec( filehdr_t *fhdrp, rv_t *rvp, char
> *path )
>   				      "%s ino %llu %s: %s: discarding\n"),
>   				      printstr,
>   				      fhdrp->fh_stat.bs_ino,
> -				      path );
> +				      path,
> +				      strerror( errno ));
>   				( void )close( sockfd );
>   				return BOOL_TRUE;
>   			}
>    


Hi Alex,

You say...
 > Based on the surrounding code, I'm sure strerror(errno) was the 
intended final argument to this call.

I'm not so sure.

The condition is comparing path length to max length, not detecting a 
previously failed system call (like the surrounding code)- as such I 
don't think the value of errno is relevant and the fix should be the 
removal of one of the format specifiers instead of the addition of the 
argument.

Here's a contrived example:

# cd /tmp
root at tulip:/tmp# mkdir mnt1
root at tulip:/tmp# mkdir mnt2
root at tulip:/tmp# dd if=/dev/zero of=block1 bs=4096 count=4096
root at tulip:/tmp# dd if=/dev/zero of=block2 bs=4096 count=4096
root at tulip:/tmp# mount -o loop /tmp/block1 /tmp/mnt1
root at tulip:/tmp# mount -o loop /tmp/block2 /tmp/mnt2
root at tulip:/tmp# socat UNIX-LISTEN:/tmp/mnt1/socket STDOUT &
root at tulip:/tmp# mv /tmp/mnt1/socket 
/tmp/mnt1/11111111112222222222333333333344444444445555555555666666666677777777778888888888999999999900000000AAAAAAAAAA
root at tulip:/tmp# xfsdump -J - /tmp/mnt1 | xfsrestore -J - /tmp/mnt2

xfsdump: using file dump (drive_simple) strategy
xfsdump: version 3.0.4 (dump format 3.0) - Running single-threaded
xfsrestore: using file dump (drive_simple) strategy
xfsdump: xfsrestore: version 3.0.4 (dump format 3.0) - Running 
single-threaded
xfsrestore: searching media for dump
level 0 dump of acer:/tmp/mnt1
xfsdump: dump date: Thu Dec 27 02:03:48 2012
xfsdump: session id: e64a2ce0-514d-47eb-8464-b3d393b11005
xfsdump: session label: ""
xfsdump: ino map phase 1: constructing initial dump list
xfsdump: ino map phase 2: skipping (no pruning necessary)
xfsdump: ino map phase 3: skipping (only one dump stream)
xfsdump: ino map construction complete
xfsdump: estimated dump size: 21120 bytes
xfsdump: creating dump session media file 0 (media 0, file 0)
xfsdump: dumping ino map
xfsdump: dumping directories
xfsdump: dumping non-directory files
xfsdump: ending media file
xfsrestore: examining media file 0
xfsrestore: dump description:
xfsrestore: hostname: acer
xfsrestore: mount point: /tmp/mnt1
xfsrestore: volume: /dev/loop0
xfsrestore: session time: Thu Dec 27 02:03:48 2012
xfsrestore: level: 0
xfsrestore: session label: ""
xfsrestore: media label: ""
xfsrestore: file system id: 59c86d2c-63c0-4ed6-a4e3-c339120db582
xfsrestore: session id: e64a2ce0-514d-47eb-8464-b3d393b11005
xfsrestore: media id: 2c893d31-7f24-4c69-a639-4464ea253683
xfsrestore: searching media for directory dump
xfsrestore: NOTE: attempt to reserve 4152 bytes for 
/tmp/mnt2/xfsrestorehousekeepingdir/dirattr using XFS_IOC_RESVSP64 
failed: Operation not supported (95)
xfsrestore: NOTE: attempt to reserve 4116 bytes for 
/tmp/mnt2/xfsrestorehousekeepingdir/namreg using XFS_IOC_RESVSP64 
failed: Operation not supported (95)
xfsrestore: reading directories
xfsrestore: 1 directories and 1 entries processed
xfsrestore: directory post-processing
xfsrestore: restoring non-directory files
xfsrestore: WARNING: pathname too long for bind of UNIX domain socket 
ino 19332 
11111111112222222222333333333344444444445555555555666666666677777777778888888888999999999900000000AAAAAAAAAA: 
(null): discarding
xfsrestore: restore complete: 1 seconds elapsed
xfsrestore: Restore Status: SUCCESS
xfsdump: media file size 21400 bytes
xfsdump: dump size (non-dir files) : 0 bytes
xfsdump: dump complete: 1 seconds elapsed
xfsdump: Dump Status: SUCCESS
root at tulip:/tmp#


unpatched xfsrestore outputs
   xfsrestore: WARNING: pathname too long for bind of UNIX domain socket 
ino 19332   
11111111112222222222333333333344444444445555555555666666666677777777778888888888999999999900000000AAAAAAAAAA: 
(null): discarding

note the "(null)".
(I fully expected this to segfault like it does on the dump file that 
caused me to notice this bug in the first place, but this being an 
example rather than 4 hours into an actual restore just before the 
holidays, it gets away with displaying "null" - how I wish it had been 
the other way around...)


with the patch as per your previous email xfsrestore outputs
   xfsrestore: WARNING: pathname too long for bind of UNIX domain socket 
ino 19332   
11111111112222222222333333333344444444445555555555666666666677777777778888888888999999999900000000AAAAAAAAAA: 
No such file or directory: discarding

But I've a feeling the "No such file or directory:" message is unrelated 
and just happens to be a left over value in errno.


whereas by removing the final specifier instead of adding the extra 
argument then xfrestore outputs
  xfsrestore: WARNING: pathname too long for bind of UNIX domain socket 
ino 19332 
11111111112222222222333333333344444444445555555555666666666677777777778888888888999999999900000000AAAAAAAAAA: 
discarding

Which I think is the correct message (with no chance of a left over 
errno value confusing it)

What do you think ?

Either way this is a minor issue now as either adding the argument or 
removing the format specifier avoids the segfault which was fatal.

Nigel