<div dir="ltr"><div dir="ltr"><br></div><br><div class="gmail_quote"><div dir="ltr" class="gmail_attr">On Mon, Feb 8, 2021 at 5:19 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/7/21 10:10 AM, Yi-Yo Chiang via Toybox wrote:<br>
> If file type is symlink and readlink() fails or returns unexpected link<br>
> size, then the file body wouldn't be written, resulting in a misaligned<br>
> archive.<br>
<br>
Yup, misaligned archive is definitely a bug. I wonder if I can reproduce it...<br>
<br>
> As explained in the comments,<br>
<br>
That's a bigger code comment than I'm checking in, FYI. (Large comments go in<br>
commit messages. In code, comments aren't executable and thus don't get<br>
regression tested, so having big ones inline is dubious unless they have a TODO<br>
in them.)<br>
<br>
> there are some cases where the link size<br>
> returned by lstat() and readlink() may be different.<br>
> To accommodate this, we readlink() and update the value of st_size<br>
> before writing the file header.<br>
<br>
I.E. the lstat() link size is not authoritative, the readlink() size is.<br>
<br>
> This may happen on Android if the userdata filesystem is encrypted.<br>
> <br>
> ---<br>
> tests/cpio.test | 2 +-<br>
> toys/posix/cpio.c | 24 ++++++++++++++++++++----<br>
> 2 files changed, 21 insertions(+), 5 deletions(-)<br>
> <br>
> diff --git a/tests/cpio.test b/tests/cpio.test<br>
> index 11b3a5bf..a12edd09 100755<br>
> --- a/tests/cpio.test<br>
> +++ b/tests/cpio.test<br>
> @@ -39,7 +39,7 @@ rm a bb ccc dddd<br>
> <br>
> # archive dangling symlinks and empty files even if we cannot open them<br>
> touch a; chmod a-rwx a; ln -s a/cant b<br>
> -toyonly testing "archives unreadable empty files" "cpio -o -H newc|cpio -it" "a\nb\n" "" "a\nb\n"<br>
> +toyonly testing "archives unreadable empty files" "cpio -o -H newc|cpio -it" "a\nb\na\n" "" "a\nb\na\n"<br>
> chmod u+rw a; rm -f a b<br>
<br>
What does this change accomplish? The changed test still passes without your<br>
code change, so what did you actually demonstrate?<br>
<br>
Ah, this fails on your broken filesystem that's returning inconsistent data. So<br>
really you just need to swap the order of a and b so the symlink has an entry<br>
after it...<br>
<br>
> diff --git a/toys/posix/cpio.c b/toys/posix/cpio.c<br>
> index 09c99ae1..200fb365 100644<br>
> --- a/toys/posix/cpio.c<br>
> +++ b/toys/posix/cpio.c<br>
> @@ -230,6 +230,7 @@ void cpio_main(void)<br>
> unsigned nlen, error = 0, zero = 0;<br>
> int len, fd = -1;<br>
> ssize_t llen;<br>
> + char *lnkbuf = NULL;<br>
> <br>
> len = getline(&name, &size, stdin);<br>
> if (len<1) break;<br>
> @@ -242,6 +243,22 @@ void cpio_main(void)<br>
> continue;<br>
> }<br>
> <br>
> + // It is possible that readlink() may fail or the actual link size is<br>
> + // different from that indicated by lstat(). This may be due to a race<br>
> + // condition (the link is removed/changed between the call to lstat() and<br>
> + // readlink()), <br>
<br>
In which case it should get caught with the lstat failure above and reported as<br>
an error before we write out a header for it. (Better to avoid writing a bad<br>
entry at all than emit nonsense for padding reasons...)<br>
<br>
> or may happen on encrypted filesystem where lstat()<br>
> + // returns the size of encrypted link and readlink() returns the size of<br>
> + // decrypted link.<br>
<br>
Still really seems like a filesystem bug to me, but we must work around the bugs<br>
that exist. (And it's not as broken as some of the crap reiserfs pulled back in<br>
the day...)</blockquote><div><br></div><div>Yeah I think this is a filesystem or kernel bug (fscrypt?) and the real good fix is for the kernel to report consistent lstat() results, but we have to remedy for existing devices out there that's running with this buggy lstat() :/</div><div>I've raised a question regarding this issue to our kernel team. </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">
<br>
> Thus we first readlink() and update the st_size, bail<br>
> + // out early if readlink() fails, then write the file header and the link.<br>
> + if (S_ISLNK(st.st_mode)) {<br>
> + lnkbuf = xreadlink(name);<br>
> + if (!lnkbuf) {<br>
> + perror_msg("readlink '%s'", name);<br>
> + continue;<br>
> + }<br>
> + st.st_size = strlen(lnkbuf);<br>
> + }<br>
> +<br>
> if (FLAG(no_preserve_owner)) st.st_uid = st.st_gid = 0;<br>
> if (!S_ISREG(st.st_mode) && !S_ISLNK(st.st_mode)) st.st_size = 0;<br>
> if (st.st_size >> 32) perror_msg("skipping >2G file '%s'", name);<br>
> @@ -262,9 +279,7 @@ void cpio_main(void)<br>
> // Write out body for symlink or regular file<br>
> llen = st.st_size;<br>
> if (S_ISLNK(st.st_mode)) {<br>
> - if (readlink(name, toybuf, sizeof(toybuf)-1) == llen)<br>
> - xwrite(afd, toybuf, llen);<br>
> - else perror_msg("readlink '%s'", name);<br>
<br>
My original bug was that should have been perror_exit() because the file was<br>
corrupted otherwise. Or the xwrite() should have been unconditional so it didn't<br>
get misaligned. Ala I _should_ have written:<br>
<br>
if (readlink(name, toybuf, sizeof(toybuf)-1) != llen)<br>
perror_msg("readlink '%s'", name);<br>
xwrite(afd, toybuf, llen);<br>
<br>
But doing the readlink with the open() and noticing failure before writing the<br>
header is the better fix, and then using the link string length to override what<br>
stat() says becomes a one line bug workaround for the new issue you hit...<br>
<br>
> + xwrite(afd, lnkbuf, llen);<br>
> } else while (llen) {<br>
> nlen = llen > sizeof(toybuf) ? sizeof(toybuf) : llen;<br>
> llen -= nlen;<br>
> @@ -276,7 +291,8 @@ void cpio_main(void)<br>
> llen = st.st_size & 3;<br>
> if (llen) xwrite(afd, &zero, 4-llen);<br>
> }<br>
> - close(fd);<br>
> + if (lnkbuf) free(lnkbuf);<br>
> + if (fd != -1) close(fd);<br>
<br>
free(0) and close(-1) are both defined as NOPs by posix.</blockquote><div><br></div><div>Good to know this!.</div><div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"> I usually test for the<br>
close one because it's an exra syscall and noise in strace, but free(0) is just<br>
a libc call. And in fact, I can just modify xclose() to do the -1 test for me...<br>
<br>
(I've gotten REALLY lax about using xclose because as far as I know the only<br>
time close() can EVER fail is on NFS, but I admit "not having to test for -1" is<br>
a reason to use it...)<br>
<br>
> }<br>
> free(name);<br>
<br>
I checked in a slightly different fix. Does that solve your problem?<br></blockquote><div><br></div><div>Yup, verified on my android test bench.</div><div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">
<br>
Thanks,<br>
<br>
Rob<br>
</blockquote></div><br clear="all"><div><br></div>-- <br><div dir="ltr" class="gmail_signature"><div dir="ltr"><table width="90%" border="0" cellspacing="0" cellpadding="0" style="margin:0px;padding:0px;font-family:"Times New Roman";max-width:348px"><tbody style="margin:0px;padding:0px"><tr style="margin:0px;padding:0px"><td style="padding:0px"><table border="0" cellspacing="0" cellpadding="0" style="margin:0px;padding:20px 0px 0px"><tbody style="margin:0px;padding:0px"><tr style="margin:0px;padding:0px"><td valign="top" style="padding:0px 20px 0px 0px;vertical-align:top;border-right:1px solid rgb(213,213,213)"><img src="https://i.imgur.com/eGpkLls.png" width="200" height="64"><br></td><td style="padding:0px 0px 0px 20px"><table border="0" cellspacing="0" cellpadding="0" style="margin:0px;padding:0px"><tbody style="margin:0px;padding:0px"><tr style="margin:0px;padding:0px"><td colspan="2" style="font-family:Arial,Helvetica,Verdana,sans-serif;padding:1px 0px 5px;font-size:13px;line-height:13px;color:rgb(56,58,53);font-weight:700">Yi-yo Chiang</td></tr><tr style="margin:0px;padding:0px"><td colspan="2" style="font-family:Arial,Helvetica,Verdana,sans-serif;padding:0px 0px 5px;font-size:11px;line-height:13px;color:rgb(56,58,53)">Software Engineer</td></tr><tr style="margin:0px;padding:0px"><td colspan="2" style="font-family:Arial,Helvetica,Verdana,sans-serif;padding:0px 0px 5px;font-size:11px;line-height:13px;color:rgb(56,58,53)"><a href="mailto:yochiang@google.com" target="_blank" class="cremed">yochiang@google.com</a></td></tr><tr style="margin:0px;padding:0px"><td colspan="2" style="font-family:Arial,Helvetica,Verdana,sans-serif;padding:0px 0px 3px;font-size:11px;line-height:13px;color:rgb(3,112,248)"></td></tr></tbody></table></td></tr></tbody></table></td></tr></tbody></table></div></div></div>