[Toybox] [landley/toybox] Segfault using toybox stat on Android emulator (#36)

[Rob Landley]

FYI, since your github bug reports are already public, I've been cc-ing
my replies to the toybox mailing list where I actually hold development
discussions and try to keep a record of stuff. I don't ever expect to
use any of Github's features except the ability to publish/mirror a repo.

For example, see:

  http://jsbackus.com/foss/fedora/2016/07/31/taking-over-a-project.html

On 08/11/2016 04:08 AM, Matthias Urhahn wrote:
> Can reproduce this with toybox 0.7.1 build against 2016.03 buildroot on
> a Nexus5 at 6.0.1.
> 
> |root at hammerhead:/sdcard # toybox_sdm stat -c "%u"
> twrp-3.0.0-0-hammerhead.img < 0 root at hammerhead:/sdcard # toybox_sdm
> stat -c "%U" twrp-3.0.0-0-hammerhead.img < Segmentation fault

The problem here isn't actually android-specific, it's that there's no
NULL check for the lookup having failed. (Oops.)

I have a better lookup function for this already in ls (which returns
the numeric ID as a string if it can't get a name, so the lookup never
fails), which is probably what I thought it was using.

> root at hammerhead:/sdcard # toybox_sdm stat -c "%g"
> twrp-3.0.0-0-hammerhead.img < 1015 root at hammerhead:/sdcard # toybox_sdm
> stat -c "%G" twrp-3.0.0-0-hammerhead.img < Segmentation fault
> 139|root at hammerhead:/sdcard # |
> 
> I would guess resolving/lookup of group id or user id (names?) causes
> the segfault.

No, failing to resolve then trying to dereference the resulting NULL
pointer anyway does.

That said, this behavior is different from ubuntu:

$ stat -c '%U %G' stat
landley landley
$ ./stat -c '%U %G' stat
 landley  landley

We're padding to 8 chars, they're not. (Why? Dunno, I think the original
submission did that.)

$ sudo chown 12345:23456 stat
...
$ ./stat -c '%U' stat
   12345
$ ./stat -c '%G' stat
   23456
$ stat -c '%G' stat
UNKNOWN
$ stat -c '%U' stat
UNKNOWN

I'd worry that "UNKNOWN" had a special meaning if I couldn't also make a
user named "UNKNOWN". :)

Ok, ubuntu isn't indenting at all. Hmmm...

$ stat /bin/ls
  File: ‘/bin/ls’
  Size: 110080    	Blocks: 216        IO Block: 4096   regular file
Device: 801h/2049d	Inode: 393412      Links: 1
Access: (0755/-rwxr-xr-x)  Uid: (    0/    root)   Gid: (    0/    root)
Access: 2016-08-12 22:16:36.184015479 -0500
Modify: 2016-03-10 13:10:57.000000000 -0600
Change: 2016-03-27 22:44:13.960855491 -0500
 Birth: -

And that's why the submitter was identing. Because the default output
does variable length indents, padding to 8 characters. Hmmm...

$ stat -c "%11G" /bin/ls
       root

Of course they did that. Why wouldn't they do that?

Right, I _actually_have_code_for_this_, it's the insanitize() function
in seq.c, and I genericized next_printf() out into lib/lib.c in case it
came up again.

The _reason_ for this insane sanitization pass is that printf has
dangerous options: %n writes to the stack, and %7p can prints the 7th
argument even when you haven't _got_ 7 arguments. (How this handles %lld
arguments I have no idea. I guess it assumes they're all integers? Added
back when everythign was promoted to "int" when it was passed on the
stack, which the people implementing 64 bits decided not to move to long
because CLEARLY that would waste too much memory and we can't have that,
because the people with 64k memory wasting 3 out of 4 bytes for a char
were SO much more resource constrained than we are today... Grrr.)

Anyway, you have to filter out possible $ and %n and so on to sanely
pass user-supplied %BLAHx infixes along to your own printf logic. Hence
the function to do that.

> It works using the stock toybox that ships with Android 6.0.1 on the
> Nexus5 (--version shows |c96e42498c99-android|):
> 
> |root at hammerhead:/sdcard # stat -c "%G" twrp-3.0.0-0-hammerhead.img root
> root at hammerhead:/sdcard # stat -c "%U" twrp-3.0.0-0-hammerhead.img root |
> 
> So this could be a toolchain issue?

Nope, another tangent entirely. :)

Working on it.

Thanks,

Rob