[Toybox] [PATCH] cpio: Implement -u (copy unconditionally)

Yi-yo Chiang yochiang at google.com
Sun Feb 28 10:29:46 PST 2021


(I'll try to keep it short)

My original motivation to send this patch is that my coworker found out
(when he was working with initramfs) that "cpio -u" behaves differently on
toybox compared to the GNU's implementation.
(my original email, in case it was flagged as spam...:
http://lists.landley.net/pipermail/toybox-landley.net/2021-February/012270.html
)
In short, I found that toybox cpio doesn't exactly behave like "-u" is
always on. I'll write the repro steps here again, since I discovered
another oddity when "-d" is specified:

(testing toybox cpio)
$ mkdir a
$ echo "a" | cpio -H newc -o >a.cpio
$ toybox cpio -iu <a.cpio
cpio: a: File exists

(toybox shouldn't report EEXIST if the directory to be created already
exist)

$ echo "a/" | cpio -H newc -o >a.cpio
$ rm -rf a
$ toybox cpio -iud <a.cpio
cpio: a/: File exists

("-d" creates the leading directory of "a/", which is "a", which causes
cpio to complain that "a" already exists when it tries to mkdir("a/")...)

My goal was to make the two cases above to not report any error (the patch
I sent only addressed the first case though),
and a follow-up question is that do we implement different behaviors with
and without "-u"?
I'm not trying to make the behavior of toybox cpio match other commands or
implementations.
Though it's a welcome change IMO if it makes subcommands behave more
consistently. For example making toybox cpio behave similar to toybox tar.


On Sun, Feb 28, 2021 at 6:41 PM Rob Landley <rob at landley.net> wrote:

> On 2/27/21 10:18 AM, Yi-yo Chiang wrote:
> > Using different command to create the archive yields different result, I
> used $
> > find dir | cpio -o ... to create the archive for testing:
> >
> > (testing the behavior of GNU cpio)
> > $ mkdir a
> > $ echo blah > a/bb
> > $ find a | cpio -H newc -o >a.cpio
> > $ # Archive a.cpio creates a (dir) and a/bb (file)
>
> It triggers on an exact match but not a mkpath. So the "create leading
> directory" plumbing doesn't do it, only if the directory is in the archive
> before the file contained in it.
>
> Were the gnu devs lazy and never bothered to properly implement -d
> combined with
> -u, or is this an intentional "design" difference?
>

I guess it's intentional? So doing something like this don't accidentally
remove "a" when the intention was to create "a/bb":
$ mkdir a
$ echo blah >a/bb
$ find a | cpio -H newc -o >a.cpio
$ rm -rf a
$ touch a
$ cpio -idu a/bb <a.cpio
cpio: `a' exists but is not a directory
cpio: a/bb: Cannot open: Not a directory
1 block

(just my speculation)


> > $ # Test dir from archive replaces existing file
> > $ rm -rf a
> > $ touch a
> > $ cpio -i <a.cpio
> > cpio: a not created: newer or same age version exists
>
> That's not something tar or zip do. Why does cpio do it? Does anybody
> depend on
> cpio doing it?
>

Probably because that was what the spec said? Quote
https://pubs.opengroup.org/onlinepubs/7908799/xcu/cpio.html : "option: u:
Copy unconditionally (normally, an older file will not replace a newer file
with the same name)."
I too would want to know who depends on this behavior. I only ever
encounter cpio archives when dealing with initramfs, and as far as I know
initramfs always replaces like "cpio -u".
Perhaps there are some use cases out there that use "cpio -p" to sync
files, thus depending on this rsync-esque timestamp checking behavior?


>
> My original idea of ignoring -u was to just do the -u behavior all the
> time,
> because that's what tar does. You're instead making it so not supplying -u
> never
> replaces files, and... I'm not sure why?
>

I assume you missed my first email
<http://lists.landley.net/pipermail/toybox-landley.net/2021-February/012270.html>.
Actually I agree we make "-u" the default and always replace everything
(for the sake of simplicity), and I was just tossing out the patch to spark
some discussion!
It's also pretty straight-forward to modify my patch to make it ignore "-u"
and make "-u" the default. Just remove the part surrounded by "if
(!FLAG(u)) {...}".


>
> According to
>
> https://www.kernel.org/doc/Documentation/filesystems/ramfs-rootfs-initramfs.txt
> (which I wrote so long ago the behavior was actually different at the time
> and I
> had to update it, but presumably if it was no longer accurate somebody
> would
> have committed a patch?) says that the kernel initramfs plumbing will
> always
> overwrite. (The original behavior is that it would append the new file's
> contents to the old file, which they decided they didn't like. :)
>

(this whole thread actually all started because I and my coworker are
working with initramfs... I did read that doc beforehand, but I never
notice the author was you until now!)


>
> > cpio: a/bb: Cannot open: Not a directory
> > 1 block
> > $ # Failed to create a (dir)
> > $ touch -d yesterday a
> > $ cpio -i <a.cpio
> > 1 block
> > $ # Interesting, if the timestamp of a (file) is older than the
> archive's a
> > (dir), then replacement happens
> > $ touch a
> > $ cpio -iu <a.cpio
> > 1 block
>
> Would always replacing and not doing this weird timestamp comparison break
> anything you know of?
>

Nothing I can think of, other than the hypothetical use case of "cpio -p" I
came up with above.
But I also won't be surprised if some decades old script may depend on
this... :/


>
> > $ # -u unlinks a (file) then mkdir(a)
> >
> > $ # Test file from archive replaces existing empty dir
> > $ rm a/bb
> > $ mkdir a/bb
> > $ cpio -i <a.cpio
> > cpio: a/bb not created: newer or same age version exists
> > 1 block
> > $ touch -d yesterday a/bb
> > $ cpio -i <a.cpio
> > 1 block
> > $ rm a/bb
> > $ mkdir a/bb
> > $ cpio -iu <a.cpio
> > 1 block
> > $ # Overall similar result when replacing existing file.
>
> What portion of this is new? What are you trying to prove?
>

I'm showing that if "a/bb" exist as an empty directory, the archive
contains a regular file "a/bb", and the existing directory has an older
timestamp, then cpio would rmdir(a/bb) before extracting "a/bb" (file).


>
> > $ # Test when a/bb is not empty
> > $ rm -rf a
> > $ mkdir -p a/bb/ccc
> > $ cpio -i <a.cpio
> > cpio: a/bb not created: newer or same age version exists
> > 1 block
> > $ touch -d yesterday a/bb
> > $ cpio -i <a.cpio
> > cpio: cannot remove current a/bb: Directory not empty
> > 1 block
> > $ cpio -iu <a.cpio
> > cpio: cannot remove current a/bb: Directory not empty
> > 1 block
> > $ # Never replaces an existing dir
>
> Does having a directory where you want to put a file come up often?
>
> What exactly is the advantage of the special case of removing an empty
> directory
> where you want to put a file? Is this a common case? Is there a use case
> for it?
> Is there any design idea here other than "match whatever gnu does"?
>

As far as I can tell not often and I don't know why GNU cpio does this
either.
My goal was to make "file replace file" and "dir replace dir" work. (ah
right I forgot to add the "dir replace dir" testcase, but one can also
argue that directory is just a special type of file...)
"dir replace file" and "file replace dir" are artifacts of the way I
implemented the patch, because I unlink/rmdir a name regardless of the file
type I'm going to create later.
I wasn't trying to match GNU's behavior, it's just that my impl happens to
behave like GNU's...


>
> Is this something tar does? Sigh. Debian's tar -x is replacing a directory
> with
> a file, but toybox's isn't. And my tar.c is dirty because of pending tar
> --transform stuff... right, one more todo item.
>
> > On Sat, Feb 27, 2021 at 3:03 PM Rob Landley <rob at landley.net
> > <mailto:rob at landley.net>> wrote:
> >
> >     On 2/20/21 8:34 AM, Yi-yo Chiang via Toybox wrote:
> >     > *ping* (in case this got categorized as spam by gmail)
> >
> >     It did, and so did this reply. :)
> >
> >     > On Sun, Feb 14, 2021 at 9:13 PM Yi-Yo Chiang <yochiang at google.com
> >     <mailto:yochiang at google.com>
> >     > <mailto:yochiang at google.com <mailto:yochiang at google.com>>> wrote:
> >     >
> >     >     If -u is specified, then replace any existing files or
> directories.
> >
> >     When does it replace directories? Do we replace a directory with a
> file, or a
> >     file with a directory? (What if the directory to be replaced with a
> file isn't
> >     empty, ala rm -r vs rmdir?)
> >
> >
> > Honestly I'm not sure, and IIUC the spec doesn't say what to do either
> > (https://pubs.opengroup.org/onlinepubs/7908799/xcu/cpio.html), so I
> assumed it
> > is implementation specific?
> > In this case I choose the behavior that is simplest to implement (IMO).
>
> Replacing an existing file with another file makes sense. Replacing a file
> with
> a directory makes a certain amount of sense because there's a common
> failure
> mode: "cp file /usr/bin" will make a file if bin doesn't exist.
>
> It's replacing a directory with a file that makes me go "huh"? For one
> thing
> "rmdir" is not nearly as commonly used as "rm -r", and mkdir -p
> path/to/file
> does not let you rmdir "path/to" (it's not empty, the single command is a
> multi-command cleanup, unless you use rm -r)...
>
> *shrug* If it's what the other one does we can do the same thing in the
> absence
> of a spec.
>
> > * If the *path* to be extracted already existed, then unlink/rmdir the
> path. *
>
> That's the problem: you can't rmdir a path you can only rmdir a single
> empty
> directory. and supporting an empty directory is like supporting a zero
> length
> file but failing if the file has any contents.
>
> > So yes we replace a file with a directory, and replace a directory (so
> long as
> > rmdir() success, which means directory must be empty) with a file.
>
> It's the "must be empty" part that strikes me as weird. Of course if we
> let it
> rm -r stuff the command immediately becomes more _dangerous_, which is
> probably
> why they don't do it. But extracting over something that disagrees whether
> files
> or directories should be there is also kinda weird...
>

Allow me to retcon:
  If the *name* to be extracted already existed, then *unlink/rmdir* the
name.

I think we are taking different mindsets here. I don't actually care if the
name to be removed is empty or not, file or directory.
My main concern is whether I can remove "name" before I extract "name".
If rmdir or unlink failed, then I'll just bailout.
"name must be empty dir" is just a requirement for rmdir to succeed.
And yes not doing "rm -rf" because it sounds like fire.



> In the absence of a spec or a consistent design idea, I tend to look at
> what tar
> and zip do and see if there's consistency there, and there sort of is but
> I need
> to go fix tar, and I never got to implement zip because stunlocked with
> other
> todo items, and I was trying to do sh.c but have been too busy to commit
> anything to that in a month, and I'm so very tired...
>
> >     Let's see...
> >
> >       $ mkdir walrus
> >       $ echo hello > walrus/file
> >       $ echo walrus/file | cpio -H newc -o > blah.cpio
> >       $ cpio -H newc -u -i < blah.cpio
> >       1 block
> >       $ mv walrus walrus2
> >       $ touch walrus
> >       $ cpio -H newc -i < blah.cpio
> >       cpio: walrus/file: Cannot open: Not a directory
> >       1 block
> >       $ cpio -H newc -u -i < blah.cpio
> >       cpio: walrus/file: Cannot open: Not a directory
> >       1 block
> >       $ cpio -H newc -dui < blah.cpio
> >       cpio: `walrus' exists but is not a directory
> >       cpio: walrus/file: Cannot open: Not a directory
> >       1 block
> >
> >     Nope, doesn't look like it. Let's see, will it replace a /dev
> node...? Yes it
> >     does (rather than writing the contents to it. What's the default tar
> behavior
> >     here... replaces the dev node. Which is not what solaris did back in
> college, I
> >     note. Why do we want cpio's default behavior to differ from tar's
> here?)
> >
> > I didn't follow.. Are you saying GNU tar replaces a device node, and
> Solaris tar
> > writes to a device node?
>
> Circa 1994 it would open whatever was there at that path, truncate it, and
> write
> to it. Truncate was ignored on a block device, so... (It was used as a way
> to
> image floppies.)
>
> The current one doesn't do that, so I guess that's the expected behavior
> now,
> which raises the question of why cpio needs -u when -u is the default for
> tar
> (and doing -u on "older" files is the default for cpio, which is another
> head
> scratcher. Seems kinda fancy given the userbase of this command...)
>

I agree it's a strange thing to do, and that's why I didn't check any
timestamp, I either replace all name (-u) or skip any existing name
(without -u).


>
> This is similar to how the initial initramfs cpio extract code would open
> the
> existing node (in O_APPEND mode instead of O_TRUNC mode) and write data to
> it.
> Not what the current one does, but I'm trying to work out the design
> reasons for
> what it's doing. (Alas posix deleted the cpio spec, presumably because RPM
> and
> initramfs are based on it and therefore Jorg Schilling felt it needed to
> die
> because Linux cooties.)
>
> > Do we want toybox cpio/tar to be feature-identical to GNU or Solaris'
> > implementation? (I hope not, cause that feels like a bloat to code size)
>
> I'm trying to figure out what the right thing to do is here. My first
> instinct
> is "if tar and zip behave the same way, cpio probably should too", but I
> don't
> want to break somebody's script.
>
> What's the use case that motivated these changes again?
>
> >     In any case, it looks like the expected behavior is to open with
> O_EXCL and
> >     unlink if it failed. Under what circumstances DOES it replace an
> existing file
> >     with a directory, or an existing directory with a file? (The tests
> you submitted
> >     only replace files with other files.)
> >
> > See my example at the start.
> > Your commands didn't replace "walrus" because your archive extracts
> > "walrus/file" but not "walrus".
>
> I know why it didn't, yes.
>
> > If you use "find ... | cpio -o" to create the test archive, then the
> existing
> > "walrus" would be replaced by cpio when cpio is trying to mkdir(walrus).
>
> Because there would then be _two_ entries in the cpio archive, and the
> directory
> entry is guaranteed to come before the directory contents by the
> definition of
> the find -depth option, which is a different test than the one I was
> running.
>
> But if you tell cpio -d it creates leading directories where needed, and in
> doing so it did _not_ remove conflicting files. And I don't understand why
> -d
> ignores -u, other than it being an oversight. (That's why the last test I
> ran
> combined -d and -u. Exact matches are treated differently than path
> components.)
>
> By the way, I suspect that zapping directories was a side effect of zapping
> symlinks, as an attack mitigation strategy. I had to care about this when
> implementing tar, but can't find the test case or blog entry. It was while
> I was
> working at Johnson Controls, I want to say early 2019? Hmmm, it wasn't
> https://nvd.nist.gov/vuln/detail/CVE-2016-6321 and I remember it was
> something
> busybox got wrong at the time...
>
> Oh great, half of tests/tar.test is just notes-to-self not actual tests.
> So much
> half-finished stuff I've never had time to finish. Well, can't do it right
> now...
>
> >     I'd like to avoid the separate "stat" if necessary not just because
> it's extra
> >     code but because it's a race condition between two non-atomic
> operations waiting
> >     to happen. (I have no idea if the gap is exploitable, but I don't
> want to worry
> >     about it if just not doing that is an option? And not doing that
> would be open
> >     O_EXCL and on failure unlink and O_EXCL a second time.)
> >
> > Yeah I totally agree. But I'd like to point out that in my first
> iteration of
> > this code, I do guard the stat() with various conditions and nested
> if-else-ifs,
>
> If all we care about is a single attempt to recover an occluded exact
> match the
> logic sounds like:
>
>   i = 0;
>   do if (-1 != (fd = open(O_CREAT|O_EXCL))) break;
>     while (!i++ && (!unlink(name) || !rmdir(name));
>   if (fd == -1) {
>     perror_msg_raw(name);
>     continue;
>   }
>
> Where does the stat come in?
>
> (Ok, if you chmod 000 a normal file the error message printed would be
> ENOTDIR
> from the failing rmdir(), technically it should fall back to the rmdir
> only if
> the unlink returns EISDIR, which the linux man page says is a non-posix
> value
> returned by Linux but the BSD man page _also_ says is what unlink(2)
> returns so
> I'm assuming mac does too.)
>
> > hoping to only call stat() if EEXIST error happened.
> > However the code quickly grew too complex and unreadable, with too many
> > combinations to consider (file replace dir, dir replace file, dir
> replace dir,
> > what if dir not empty? what about symlink and special files...), so I
> changed
> > the implementation to the one you're seeing now.
> > This is also the reason why I add the "if dir already exist, then just
> do chmod"
> > shortcut, because race could happen between rmdir(a) and mkdir(a).
>
> A race resulting in a failure, you mean?
>

I mean another process may create "a" between rmdir(a) and mkdir(a).
Second thoughts, this is not a real issue because that would just make
mkdir(a) to fail with EEXIST...
I should just say to save an extra syscall. (rmdir(a) + mkdir(a) vs
chmod(a, ...))


>
>   $ ln -s broken nowhere
>   $ ls -l nowhere
>   lrwxrwxrwx 1 landley landley 6 Feb 28 03:09 nowhere -> broken
>   $ mkdir nowhere
>   mkdir: cannot create directory ‘nowhere’: File exists
>
> How is this exploitable?
>
> The reason I'm spending time on this thread instead of staring at the cpio
> code
> is that implementation is easy, figuring out what it SHOULD do is hard. I
> still
> don't know what use case motivated your changes. Your patch description
> says
> what but not why, I have to reverse engineer an understanding of what
> you're
> trying to accomplish from the conclusion you came to about the best way to
> implement it.
>
> > Another point is that when replacing an existing file, having a separate
> stat()
> > would mean:
> > stat(a) & unlink(a) & open(a, O_EXCL)
> > but not stat()-ing beforehand would mean:
> > open(a, O_EXCL) [fail] & stat(a) & ("a" is a directory ? rmdir :
> unlink)(a)
> > & open(a, O_EXCL)
>
> Isn't the open(a, O_EXCL) succeeding the common case though? And if that's
> the
> first thing it tries and it succeeds...
>
> > So having a separate "stat" doesn't necessarily mean an extra system
> call.
> > If path doesn't exist then the stat() is an extra syscall, otherwise the
> stat()
> > saves an extra syscall.
>
> Mostly it's just the possibility that the thing I statted is not the thing
> I
> acted upon a moment later making me shy away from it as a first choice for
> implementation. I prefer not to have to reason about what _could_ happen
> in that
> gap if there's another way (such as the open saying it didn't work and then
> unlink saying it didn't work so you have to rmdir; the first common case
> didn't
> succeed and the second common case didn't succeed so then try the third.
> Does a
> failed unlink attempt take longer than a stat?)
>

Both work for me. I simply chose a different implementation. I don't know
the overhead of stat() / unlink() though... I always assumed them to be
amortized insignificant due to the file system caching the inode?


>
> I note that the obvious approach is to wrap the whole if/else staircase
> with a
> retry loop (otherwise there's a goto), which makes the patch look big
> because of
> the extra indentation layer (which is why git diff supports -b, and yes
> I've
> done that on patches to see what people actually did...)
>
> >     (You'll notice the existing stat was an fstat() on an already open
> file
> >     descriptor, and you're adding an lstat on a path which could get
> swapped out
> >     afterwards. In general I try not to do that because it seems racy
> with programs
> >     dropping symlinks based on inotify triggers. Yeah you've sprayed the
> system down
> >     with selinux but I don't want to worry about it at a design level...)
> >
> >     Unfortunately I haven't had the spare focus this week to properly
> wrap my brain
> >     around what I want to happen here. I finally got a chance to look at
> it the end
> >     of the day friday, and just wanted to say why I haven't applied it
> as-is yet.
> >     I'll try to redo it this weekend, but first I need to confirm that
> open(O_EXCL)
> >     is sufficient error detection and what we do about conflicting
> directory
> >     creation (is there a case where -u DOES apply to -d?)
> >
> > I hope not, let's keep it simple...
>
> It looks like only exact matches of empty directories get rmdir()ed (and
> really
> it's unlinkat(AT_REMOVEDIR) and why can unlink remove files, dev nodes,
> symlinks, sockets, and fifos, but directories have their own remove call
> (even
> though that call fails if the directory isn't empty)? Why is that again?
> I'm too
> tired for this to make sense right now...
>
> >     and adding tests for the
> >     corner cases limning in the design decisions.
> >
> > I can add more tests once we decide the desired behavior.
> Tests are fiddly. I really HATE when TEST_HOST gives spurious
> irreproducible
> failures:
>
> FAIL: tar sparse without overflow
> echo -ne '' | tar c --owner root --group root --mtime @1234567890 --sparse
> fweep
> | SUM 3
> --- expected    2021-02-28 09:28:39.518369796 +0000
> +++ actual      2021-02-28 09:28:39.526369797 +0000
> @@ -1 +1 @@
> -e1560110293247934493626d564c8f03c357cec5
> +ce3f8e6b10723ce4d85f06c606bc634a9db23be0
> .singlemake:1781: recipe for target 'test_tar' failed
> make: *** [test_tar] Error 1
>
> Smells like maybe clock edge overflow of a second ticking over and winding
> up
> altering a header field? But I didn't grab the artifacts out of generated/
> to
> see what it was and it refuses to do it again...
>
> Anyway, added a new test to tar and made it unlink an empty directory
> where it
> wants to put a file. If the result of all this is "cpio should work like
> tar
> when files/directories conflict with archive contents" then that's easy to
> implement. If not, I'd like to understand _why_ not.
>
> >     I need to properly wrap my head around this, but haven't been at my
> best the
> >     past couple weeks. Slightly overstretched. Sorry 'bout that.
> >
> > Nah it's cool. Do take care and take your time and don't feel burned
> out.
>
> Too late, but it's not because of this.
>
> I _should_ just sit down and work out what to do and implement it instead
> of
> having email threads that boil down to "I haven't scraped up enough brain
> to
> work through all these corner cases myself yet", but I've been tired and
> headachey for a few days now. I'd blame cedar pollen but my wife (in
> minneapolis) has been nauseous for 2 days and apparently it's going around?
>
>   https://twitter.com/catacalypto/status/1365538065184251905
>
> To be honest I expect I'm still limp from the blizzard last week, which I
> _shouldn't_ be but 2020 kinda burned through my reserves. Just because the
> people who tried to violently overthrow the government last month have
> deregulated infrastructure in my state to the point civilization literally
> collapsed around here last week, and yesterday a jewish friend broke down
> sobbing because the CPAC conference's stage is literally shaped like a nazi
> symbol...
>
> But there's good news: my house _didn't_ get flooded this time (after the
> first
> 4 times my wife has outright PTSD about that but she's been getting her
> doctorate in another state for years and we haven't been able to see each
> other
> in person since the pandemic started). The boil water advisory here was
> lifted
> all the way back on wednesday, our power company isn't one of the ones
> charging
> 5 figure electricity bills (instead they say they'll spread it out over
> the next
> 10 years), and as of yesterday the local grocery store has restocked over
> half
> its shelves. Plus the Johnson and Johnson one dose vaccine got approved,
> which
> is the one I've been rooting for since brexit derailed availability of the
> oxford one. If everything goes well I'm hoping I can leave the house and go
> inside another building (other than the grocery store down the street) by
> maybe
> september. (I still have the phobia of needles resulting from a nurse
> wheeling
> in a tray of 24 needles for allergy testing when I was 7 and tying me up
> in a
> sheet when I freaked out about it until I threw up on her. Ever since I've
> been
> able to make my gums bleed by thinking to hard about needles, but I can
> usually
> psych myself up to get an injection without losing consciousness given
> about
> half an hour. So I have that to look forward to, but this is part of the
> reason
> I really want the one dose version instead of the two dose version...)
>
> Anyway, weekend! In theory this means spare cycles to throw at the toybox
> pull
> request and bug report backlog (instead of working on the shell I've been
> trying
> to finish, but nobody except me uses that and other people use the stuff
> they're
> sending me patches for, so...)
>
> Rob
>
> P.S. Sorry I haven't been more focused. And Pascal's Apology about the
> length of
> the email:
>
> https://www.npr.org/sections/13.7/2014/02/03/270680304/this-could-have-been-shorter
>


-- 

Yi-yo Chiang
Software Engineer
yochiang at google.com
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.landley.net/pipermail/toybox-landley.net/attachments/20210301/a5f15f7b/attachment-0001.html>


More information about the Toybox mailing list