[Toybox] [PATCH] cpio: fix -it

[Rob Landley]

On 03/29/14 00:46, Isaac Dunham wrote:
> On Thu, Mar 27, 2014 at 07:10:45AM -0500, Rob Landley wrote:
>> On 03/26/14 12:13, Isaac Dunham wrote:
>>> Fix cpio -it: don't close(fd) unless we opened it.
>>
>> Ooh, good catch. (Applied.)
>>
>> It's hard to get the code right without a good test case, and with
>> aboriginal in pieces at the moment (ccwrap rewrite, switching over to
>> musl) I haven't got a convenient chroot set up and am reluctant to test
>> stuff as root on my work system (without which it can't create /dev
>> nodes, fifos, and so on).
> 
> Yes...
> All the stuff I've found has been by way of testing with
> <generate list of files> |cpio -o |cpio -it
> and compare the results to the original list of files.
> While that doesn't necessarily test the format compatability, it's much
> more likely to fail when either writing or reading invalid archives than
> no test cases.

I was comparing against an aboriginal root filesystem cpio archive (from
a SYSIMAGE_TYPE=initramfs build). I plan to plug in toybox cpio to
generate that (instead of the snapshot of the kernel tool that's doing
it now), but that's a todo item that involves adding the ability to
parse/consume the kernel file list format mentioned earlier.

> Now, as far as files to look at...
> We want a list of files with names that are 1, 2, 3, and no bytes longer
> than n*4 bytes to check padding; at least one unopenable device, socket,
> symlink, or directory; and ideally, a selection of file sizes.
> Without root access, /dev/console counts as unopenable.

Cut and pasted straight into my todo list. :)

> 
>> Also, I still have todo items. It should probably use the uid/gid fields
>> when running as root, for example. (Create preserves 'em, but extract
>> doesn't set them.)
> 
> Which will be roughly:
> 
>   if (!(toys.optflags & FLAG_no_preserve_owner) && (!geteuid())
>     chown(name, x8u(toybuf+22), x8u(toybuf+30));
>   errno = 0;
> 
> Not tested yet, but trivial.

Except I'm worried about the race window of "mknod a file, set the suid
bit on it" or similar. In cp I did a lot of careful sequencing to try to
minimize that, which was why all the mknodat() and fchmod() and such. I
don't know if any of that can be genericized or shared, but I should
probably look into it.

That said, in this instance the only time chown() matters is when you do
it as root, in which case the file was already created as root, so i
this case...

(The NSA wrote most of selinux. The NSA has come out of the closet as
_not_ wanting to close vulnerabilities, but instead keep systems
vulnerable. I infer a relationship between these two data points.)

Sigh. --no-preserve-owner. It would be really NICE if there was some
variant of a current standard for this command. I need to start engaging
with The Austin Group and going "next posix: fixit!" about all sorts of
things...

> Anyhow, I've got some bits of cleanup for openvt and deallocvt, and 
> help updates for find, since I've had no net for a couple of rainy days.
> 
> Speaking of help... I notice that www/help.html did not actually get added;
> the reference was all that showed up.

I put it on the website, but didn't check it in since it's a generated
file I need to regenerate each release. It's just:

  make defconfig && ./toybox help -ah > www/help.html

Which I noted in the checking comment. I'm open to suggestions about a
better place to put that. (FAQ entry maybe?)

Rob

 1396127321.0