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

[Yi-yo Chiang]

(I kind of dozed off... so break off some questions / answers to another
mail)

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

Answered in the previous reply.


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

I think tar doesn't remove conflicting leading directories either? Not that
this means anything, but if we want to make cpio be similar to tar then we
would want to keep this behavior.


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

In such case a separate "stat and unlink/rmdir" would produce a better
error message?


>
> > 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?
>
>   $ 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?)
>
> 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...
>

Why do rmdir() and unlink() have to be two different calls?
I have the same question ever since college, and it's still puzzling me
today. Over the years I learned to just live with it and stop bugging other
people with this kind of question. I guess this is what socialization feels
like.


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

Mail is long, and I kind of lose track of what the topic is again? Anyway
if this (should cpio be similar to tar in resolving name conflicts?) is
ever a question, then my answer would be "yes, why not?".
Ah right, the problem is that cpio (toybox) right now only replaces a
conflicting name when both the name to be extracted and the name to be
replaced are regular files.


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

Can't say I understand, because I don't live in the states, thus saying
that would sound like a hypocrite.
I'm from a country that doesn't have that big of a pollen issue (I think
the trees here are different?), we always boil our water even pre-COVID, no
blizzard but typhoons, and we don't know what's a COVID vaccine :)
I think rest more, work less, and eat balancedly and perhaps headaches
would go away? (But who am I to say, I'm working too much already...)



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

^^^ I didn't finish this "apology" either, it's still too long!

-- 

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/9578945c/attachment-0001.html>