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

Tue Mar 2 09:56:03 PST 2021

On Mon, Mar 1, 2021 at 5:43 PM Rob Landley <rob at landley.net> wrote:

> On 2/28/21 12:29 PM, Yi-yo Chiang wrote:
> > (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.
>
> "What was the actual problem?" "My coworker noticed this existed."
>
> That's... not a use case?
>

from the bugs, i think the original bug was the "File exists" -d bug from
the ramdisk cpio file, and they hit the -u behavior difference trying to
work around that before realizing that -d was the real problem? (or maybe
-u was an attempt to produce a small repro case for the -d bug. Or both!)

i'll sync AOSP today anyway. thanks!

> > (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.
>
> Not behaving exactly like gnu is not in itself a bug. (An undocumented
> deviation
> from posix is, but a lot of times the fix is documenting it in the
> "deviations
> from posix" section of the command's source...)
>
> > 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)
>
> Since I can't get a clear explanation of what you want, I just implemented
> the
> simple thing I wanted to do at the start. Commit f1be076b52ad.
>
> By the way, when I trimmed down your patch file to add just the tests, I
> found
> out that if you leave the trailing
>
>   diff --git a/toys/posix/cpio.c b/toys/posix/cpio.c
>   index 31c777c9..795f890c 100644
>
> lines of a hunk but delete the rest debian's patch will announce that it's
> patching that file, do nothing, and not have any sort of error about it. I
> don't
> think mine does that. Is mine behaving differently here also a bug by
> definition
> because it doesn't match the gnu behavior exactly?
>
> Oh, and in your patch:
>
>      } else if (S_ISLNK(mode)) {
>        data = strpad(afd, size, 0);
> -      if (!test) err = symlink(data, name);
> +      if (!test) {
> +        err = symlink(data, name);
> +        // Can't get a filehandle to a symlink, so do special chown
> +        if (!err && !geteuid() && !FLAG(no_preserve_owner))
> +          err = lchown(name, uid, gid);
> +      }
>        free(data);
> -      // Can't get a filehandle to a symlink, so do special chown
> -      if (!err && !geteuid() && !FLAG(no_preserve_owner))
> -        err = lchown(name, uid, gid);
>
> err is initialized to zero each time through the loop, nothing sets it
> before
> the call to symlink that is skipped by if (!test), so when !test the if
> (!err)
> has to trigger, that's why it didn't have to be part of the earlier block.
> (Yes
> I read through your patch to see if you found any actual bugs you didn't
> mention, that's why this is so much work. I want to understand what's
> going on.)
>
> > $ 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/")...)
>
> Same error with and without -d so the problem is -d?
>
> Suppressing mkdir errors with -u should also coincidentally fix this.
>
>
> > My goal was to make the two cases above to not report any error (the
> patch I
> > sent only addressed the first case though),
>
> Should your most recent case still set the existing directories to the
> ownership, permissions, and timestamp from the archive? Or just silently
> ignore
> the conflict?
>
> You are not expressing a coherent design idea. What should it do and why?
>
> The OTHER thing my commit doesn't handle is if an undeletable file exists
> where
> a directory should go (owned by another user for example) it won't report
> error,
> but the common use case of cpio is to mkdir and then put something in the
> dir
> and the later one would still barf. (I check for EEXIST but even the man
> page
> says "EEXIST pathname already exists (not necessarily as a directory)." so
> that
> wouldn't help.)
>
> FYI, I'm more comfortable with "optionally attempt to clear a path and
> report
> failure only on later attempts to use it" than "rely on stat's contents in
> a
> stat-wait-open usage pattern to mean anything about what got opened"
> because the
> second is a common security pattern that there are static analyzers
> searching
> for to exploit, and the first isn't that I'm aware of. The failure case
> when -u
> doesn't work is "act as if they didn't supply -u", which seems reasonable
> to me.
>
> > and a follow-up question is that do we implement different behaviors
> with and
> > without "-u"?
>
> I wasn't doing so, I was ignoring -u.
>
> If the complaint is "tar and zip behave one way but cpio doesn't" I find
> that a
> compelling argument. If "gnu cpio needs -H newc to produce usable archives
> but
> toybox does so by default, so change toybox to require -H newc even though
> it
> can't produce any other archive format because otherwise their behavior
> differs", I do not find that a compelling argument. A difference is not
> inherently a bug.
>
> A difference that breaks existing code that works in one context but does
> not
> work in another is a bug, even what that difference is clearly stupid and
> we
> need "bug compatibility" to get there.
>
> My go-to example is commit 32b3587af261 where perl had a miswritten NOP sed
> expression that gnu silently ignored but mine reported as an error. What
> the
> perl devs wrote clearly wasn't what they MEANT, and if there had been an
> error
> they would have spotted this bug and fixed the regex before they shipped.
> But
> they didn't, so I changed mine to accept the bug so it could run the perl
> build,
> even though the resulting behavior was "wrong" something depended on it.
>
> > I'm not trying to make the behavior of toybox cpio match other commands
> or
> > implementations.
>
> Then what ARE you trying to do? (Do you have a use case?)
>
> > Though it's a welcome change IMO if it makes subcommands behave more
> > consistently. For example making toybox cpio behave similar to toybox
> tar.
>
> That was my original idea, but tar has -k to switch it _off_ which cpio
> does not
> have, and inventing a new option means that a toybox script using it would
> break
> with "illegal option" when run with busybox's version...
>
> (If you want "portable", don't have conflicting files where you're
> extracting
> to. There's already version skew in the kernel's initramfs extractor about
> that...)
>
> Anyway, patch checked in. I'm gonna stop the thread here unless you want
> to send
> me more test cases.
>
> Rob
> _______________________________________________
> Toybox mailing list
> Toybox at lists.landley.net
> http://lists.landley.net/listinfo.cgi/toybox-landley.net
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.landley.net/pipermail/toybox-landley.net/attachments/20210302/f4db6eca/attachment-0001.htm>