xfs
[Top] [All Lists]

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

To: Alex Elder <elder@xxxxxxxxxxx>
Subject: Re: [PATCH] xfsdump: fill in missing strerror() call
From: Nigel Tamplin <ntamplin@xxxxxxxxxxxxxxx>
Date: Thu, 27 Dec 2012 03:16:58 +0000
Cc: xfs@xxxxxxxxxxx
In-reply-to: <50DB794F.7010906@xxxxxxxxxxx>
References: <50D7A1C2.9000609@xxxxxxxxxxxxxxx> <50DB794F.7010906@xxxxxxxxxxx>
User-agent: Mozilla/5.0 (X11; U; Linux x86_64; en-US; rv:1.9.1.16) Gecko/20121027 Icedove/3.0.11
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@xxxxxxxxxxx>
Reported-by: Nigel Tamplin<ntamplin@xxxxxxxxxxxxxxx>
---
  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@tulip:/tmp# mkdir mnt1
root@tulip:/tmp# mkdir mnt2
root@tulip:/tmp# dd if=/dev/zero of=block1 bs=4096 count=4096
root@tulip:/tmp# dd if=/dev/zero of=block2 bs=4096 count=4096
root@tulip:/tmp# mount -o loop /tmp/block1 /tmp/mnt1
root@tulip:/tmp# mount -o loop /tmp/block2 /tmp/mnt2
root@tulip:/tmp# socat UNIX-LISTEN:/tmp/mnt1/socket STDOUT &
root@tulip:/tmp# mv /tmp/mnt1/socket /tmp/mnt1/11111111112222222222333333333344444444445555555555666666666677777777778888888888999999999900000000AAAAAAAAAA
root@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@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

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