Re: [PATCH] bsdcpio core dump

Thank you!  I found this bug last week but didn't
know there was such an easy way to trigger it, so
I haven't been in much of a hurry to commit the fix.
I'll try to get that taken care of tonight.

This turned out to be a libarchive bug; write_header() is
returning the wrong code.  ARCHIVE_WARN is supposed
to indicate that the operation "succeeded with caveats".
In particular, ARCHIVE_WARN from write_header() indicates
that it's safe to write the following data.
Libarchive should be returning ARCHIVE_FAILED in
this case, to indicate that this particular file
has to be abandoned.

In particular, exit(1) is the wrong response to this
particular error.  Subsequent files might be going into
other directories, after all, so cpio should keep on going
and try the next file.

The error messaging here is rather muddy and this
duplication of messages has been bugging me for a while.
I think the best solution is to go inside of libarchive
and strip out the code that appends the strerror()
information there, on the logic that the errno value
is available to clients that want to display it.
Then archive_error_string() will just contain the
libarchive-specific clarification, so it will be appropriate
to emit both.

Tim


Giorgos Keramidas wrote:
> Hi Tim & -CURRENT,
> 
> I think I stumbled upon a minor bsdcpio bug.  When bsdcpio tries to
> write to a directory it doesn't have permissions it core dumps:
> 
> # mkdir /tmp/koko
> # chmod 0700 /tmp/koko
> 
> % cd /ws/keramida/src/usr.in/cpio
> % env DEBUG_FLAGS='-ggdb' CFLAGS='' make bsdcpio
> ...
> % echo bsdcpio | ./bsdcpio -p -dmvu
> bsdcpio: /tmp/koko/bsdcpio: Can't create '/tmp/koko/bsdcpio': Permission denied: Permission denied
> INTERNAL ERROR: Function 'archive_write_data' invoked with archive structure in state 'header', should be in state 'data'
> Segmentation fault: 11 (core dumped)
> 
> % gdb bsdcpio bsdcpio.core
> #0  0x280b9ad7 in diediedie () at archive_check_magic.c:55
> 55              *(char *)0 = 1; /* Deliberately segfault and force a coredump. */
> (gdb) bt
> #0  0x280b9ad7 in diediedie () at archive_check_magic.c:55
> #1  0x280b9caf in __archive_check_magic (a=0x28202140, magic=3221336261, state=4, 
>     function=0x280bc960 "archive_write_data") at archive_check_magic.c:116
> #2  0x2809f95d in _archive_write_data (_a=0x28202140, buff=0x804f360, size=16384) at archive_write_disk.c:625
> #3  0x280a2e35 in archive_write_data (a=0x28202140, buff=0x804f360, s=16384) at archive_virtual.c:74
> #4  0x0804b534 in entry_to_archive (cpio=0xbfbfe6ec, entry=0x282231c0) at cpio.c:637
> #5  0x0804b219 in file_to_archive (cpio=0xbfbfe6ec, srcpath=0x2821e000 "bsdcpio") at cpio.c:542
> #6  0x0804c1d1 in mode_pass (cpio=0xbfbfe6ec, destdir=0xbfbfe94e "/tmp/koko") at cpio.c:939
> #7  0x0804a899 in main (argc=4, argv=0xbfbfe7c8) at cpio.c:291
> (gdb) 
> 
> It seems that cpio tries in entry_to_archive() to handle errors in calls
> of archive_write_header() but it should be a bit more strict about it.
> I picked up ARCHIVE_RETRY as the smallest error code we 'accept' when
> archive_write_header() fails, but the choice seems pretty arbitrary.
> The patch does solve the core dump I was seeing though.  Does it look
> good enough for cpio?  Should we handle archive_write_header() errors
> differently?
> 
> %%%
> diff -r cb9a95f8dfb3 usr.bin/cpio/cpio.c
> --- a/usr.bin/cpio/cpio.c	Tue Jan 20 21:45:52 2009 +0200
> +++ b/usr.bin/cpio/cpio.c	Tue Jan 20 22:56:48 2009 +0200
> _at__at_ -623,12 +623,12 _at__at_
>  	r = archive_write_header(cpio->archive, entry);
>  
>  	if (r != ARCHIVE_OK)
> -		cpio_warnc(archive_errno(cpio->archive),
> +		cpio_warnc(0,
>  		    "%s: %s",
> -		    destpath,
> +		    srcpath,
>  		    archive_error_string(cpio->archive));
>  
> -	if (r == ARCHIVE_FATAL)
> +	if (r < ARCHIVE_RETRY)
>  		exit(1);
>  
>  	if (r >= ARCHIVE_WARN && fd >= 0) {
> %%%
> 
> While here, I also changed destpath to srcpath in the warning and
> changed the 'code' argument of cpio_warnc() to zero, to avoid a
> duplicate strerror() like output.  The message returned by
> archive_error_string() contains the destination path and strerror() text
> already, so this changes the error from:
> 
> bsdcpio: /tmp/koko/bsdcpio: Can't create '/tmp/koko/bsdcpio': Permission denied: Permission denied
> 
> to:
> 
> bsdcpio: ./bsdcpio: Can't create '/tmp/koko/bsdcpio': Permission denied
> 
> The cosmetic changes can probably go in a separate commit, but we should
> probably fix & backport the core dump when archive_write_header() fails.
> 
> 
>