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