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

Wed Dec 26 21:36:46 CST 2012

On 12/26/2012 09:16 PM, Nigel Tamplin wrote:
> 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.

Next time I'll actually read the code.

Yes, I fully concur with your analysis.

Do you want to submit a patch?  Or do you want me to re-do mine?

					-Alex

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