[Toybox] Android sitrep

[Rob Landley]

On 10/17/19 4:09 PM, Denys Nykula wrote:
>> the tar "no ../ files outside this directory" check is
>> false positiving on "./" for some reason, gotta track that down.
> 
> Here's a reproducible toy tar crash, might or might not be connected.

I wasn't seeing a segfault.

> git clone https://github.com/landley/toybox
> wget http://musl.cc/x86_64-linux-musl-native.tgz
> rm -r root.tar toybox/root x86_64-linux-musl-native
> (cd toybox; make defconfig root)
> toybox/toybox tar xf x86_64-linux-musl-native.tgz
> (cd toybox/root/*/*fs; ../../../toybox tar c *)>root.tar
> cd x86_64-linux-musl-native &&../toybox/toybox tar xvf ../root.tar
> # ...
> # tar: can't remove: bin: Is a directory
> # tar: can't remove: lib: Is a directory
> # tar: 'usr/' not under '/path/to/x86_64-linux-musl-native'

Hmmm, the usr->. is confusing it because "usr" _isn't_ under that directory, it
_is_ that directory. (It's not a child of it, it's the thing itself.)

That's... technically correct? Although it's more a NOP than an error in this
corner case.

> # Segmentation fault

Sigh. Reproduced.

It's because sbin is a circular symlink: you have sbin -> usr/sbin and usr -> .
so the link points back to itself. This makes xabspath() return null and I'm
calling it in a mode where the "x" part doesn't error_exit for that, but didn't
handle it myself. :P

Huh. What _is_ the right behavior for this? There's _kinda_ some conflicting
assumptions in these two filesystems. :)

> First two errors are correct, refusing to overwrite an existing dir with
> a symlink and moving on, remembering to make a bad exit code later. But
> then you hit an existing symlink to a dir. Other implementations replace
> the symlink with the dir and keep unpacking files there. Toy tar doesn't
> overwrite, says odd message, starts extracting children of a dir that
> hasn't been created, and crashes. Full output below.

That's not why it crashes, it crashes because it couldn't resolve the absolute
path and I called an xfunction() in a mode that didn't error_exit() and then
didn't handle the error myself. :)

Probably something like:

--- a/toys/posix/tar.c
+++ b/toys/posix/tar.c
@@ -392,7 +392,11 @@ static int dirflush(char *name)

   // Barf if name not in TT.cwd
   if (name) {
-    ss = s = xabspath(name, -1);
+    if (!(ss = s = xabspath(name, -1))) {
+      error_msg("'%s' bad symlink", name);
+
+      return 1;
+    }
     if (TT.cwd[1] && (!strstart(&ss, TT.cwd) || *ss!='/')) {
       error_msg("'%s' not under '%s'", name, TT.cwd);
       free(s);

The question is, what behavior do you want? If sbin points to usr/sbin and usr
points to . then sbin points to sbin and you _can't_ extract stuff under it.
What behavior do you expect here?

It looks like the gnu/dammit tar is deleting the usr symlink and replacing it
with a directory. I can do that for directories I guess? (Fiddle, fiddle...)
Yes, it looks like it's _always_ replacing a symlink with a directory when
extract says a directory should be there. Which is a little awkward because the
"is this target under here" test comes _before_ it looks at _what_ should go
there, so I need to rearrange things a bit when I'm more awake.

Rob