<div dir="ltr"><div dir="ltr"><br></div><br><div class="gmail_quote"><div dir="ltr" class="gmail_attr">On Mon, Oct 25, 2021 at 8:34 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 10/25/21 4:53 PM, enh wrote:<br>
> On Thu, Oct 21, 2021 at 12:18 AM Rob Landley <<a href="mailto:rob@landley.net" target="_blank">rob@landley.net</a><br>
> On 10/20/21 4:35 PM, enh wrote:<br>
> > if you don't mind creating a /system directory (or symlink) in /, you can just<br>
> > run like that. there's a script in AOSP bionic that sets up all the links you<br>
> > need to run the bionic tests on the host.<br>
> <br>
> [Removed flailing trying to find the script, you said where it is later.]<br>
> <br>
> Does it fix the OTHER warnings about being unable to "__bionic_open_tzdata:<br>
> couldn't find any tzdata when looking for utc!" and such? I still need to fix<br>
> the "FAIL: find -type f -user -exec" because bionic can't read my $USER out of<br>
> /etc/passwd...<br>
> <br>
> Ahem:<br>
> <br>
> env: exec env: No such file or directory<br>
> PASS: env -i<br>
> <br>
> Right, you disabled builtins, <br>
> <br>
> yeah, to try to reduce mksh/toybox confusion. if/when we switch /bin/sh to<br>
> toybox it should reduce compat issues too.<br>
<br>
Doing so uncovered a legitimate bug (which I've been too buried to address but<br>
it's on the todo list) and made me think I might need to run the test suite with<br>
every combination of CFG_TOYBOX_* toggled, which is exponentially increasing on<br>
the number of options. :(<br></blockquote><div><br></div><div>it's definitely useful to at least test the "all toybox" configuration, since that's basically what we test on Android (modulo mksh as the shell). but i catch anything like that within a week or so anyway :-)</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">
> and there's no $PATH. I should $(which env) there.<br>
> And maybe add an || echo fail<br>
> <br>
> (And now having done the $(which) annd checked it in... "env -i env" on the host<br>
> does NOT have this failure. The -i does not apply to env's own exec. Sequencing<br>
> I need to fiddle with...)<br>
> <br>
> All the other failures seem to be the unicode plumbing.<br>
> <br>
> > (And when I _tried_ to create a chroot as a normal user, bionic segfaulted<br>
> > before calling main() because /dev/null wasn't there. Did that ever<br>
> get fixed?<br>
> > Hard to run an init linked against bionic when that's the case.<br>
> ><br>
> > have you tried? i'm pretty sure there's a special case for pid 1, for exactly<br>
> > this reason :-)<br>
> <br>
> I'm going to give you a moment to think about what you did.<br>
> <br>
> yeah, it's one of those things that's hard to reason about ---<br>
<br>
I was wincing that you'd added a special case to work around a bug, and in the<br>
non-special case used a segfault as a security mechanism. (Although rereading<br>
bionic/libc_init_common.cpp it's more explicit/intentional about it all than I<br>
remembered.)<br>
<br>
> "does my<br>
> tiger-repellant bracelet actually work, or are there just no tigers where i<br>
> live?".<br>
<br>
As Alice asked the White Knight in "Through the Looking Glass".<br>
<br>
> specifically "has Android never had a stdio confusion CVE because of<br>
> this, or because the world had already moved on from those security bugs [which<br>
> were all the rage when Android started], or just because we don't have anything<br>
> like inetd?".<br>
<br>
Or because you're didn't have an NDK for the first several years and third party<br>
apps are almost entirely GUI programs that don't really interact with a text<br>
console?<br>
<br>
What stops code from losing track of filehandles and calling close(1) and then<br>
opening a new stdout a moment later by accident? I remember segfaulting one of<br>
the gnu tools feeding it "-" as an argument more than once (it was a "what<br>
happens if" test implementing the toybox version and the answer, as is so often<br>
the case, is "nobody in gnu-land ever thought of that or tried it").<br></blockquote><div><br></div><div>fdsan: <a href="https://android.googlesource.com/platform/bionic/+/master/docs/fdsan.md">https://android.googlesource.com/platform/bionic/+/master/docs/fdsan.md</a><br></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">
This is why toybox has notstdio() and xopen_stdio() and such in lib/xwrap.c to<br>
handle this sort of thing itself on _every_ open. (xopen_stdio() being the<br>
version that DOESN'T dup() any filehandle <3 and backfill with /dev/null until<br>
it's returning fd 3 or greater.)<br>
<br>
> hard to untangle those, and since security -- like law -- is an<br>
> ass-covering business, no-one wants to be the one to _remove_ anything, even if<br>
> it's not obvious that it's really helping. (and in this case it's not obvious<br>
> that it's _not_ helping, so...)<br>
<br>
The specific problem I had was:<br>
<br>
$ x86_64-linux-android-cc --static hello2.c<br>
$ mkdir ch<br>
$ mv a.out ch<br>
$ sudo chroot ch ./a.out<br>
Segmentation fault<br>
<br>
The new bionic process _had_ an existing stdin/stdout/stderr, and yet segfaulted<br>
because the startup code tried to open("/dev/null") before it even looked at the<br>
three filehandles. The sanitizer had nothing to do and did it wrong. (Failed<br>
doing something it would then immediately undo without using.)<br>
<br>
If instead the whole fixup function was just:<br>
<br>
for (i = 0; i<3; i++)<br>
if (-1 == fcntl(i, F_GETFL) && errno == EBADF) open("/dev/null", O_RDWR));<br>
<br>
Then:<br>
<br>
A) Posix says open() returns lowest fd available so it has to backfill 0/1/2, no<br>
dup() required.<br>
<br>
2) Defers the open() until there's an actual problem. Yes it calls open more<br>
than once if it has more than one thing to do (instead of dup) but it's one call<br>
made in a loop, and doesn't happen at all in the common case.<br></blockquote><div><br></div><div>yeah, that sounds reasonable. done in <a href="https://android-review.googlesource.com/c/platform/bionic/+/1869594">https://android-review.googlesource.com/c/platform/bionic/+/1869594</a>.</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">
III) I didn't bother checking for error because inability to open "/dev/null" is<br>
a "should never happen" in a function already attempting to correct for a<br>
"should never happen", which then may or may not cause an actual problem later<br>
on if it hadn't been corrected. Six sigma meets a billion unit volume means you<br>
hit 3400 failures so I understand the paranoia, but this is two levels of pilot<br>
error to reach a maybe. However if you really want to error check the deferred<br>
open, doing so wouldn't breaking a chroot that DOES have stdin/out/err.<br>
<br>
*) If you DO error you'd still need to special case PID 1 to support the<br>
initramfs with no /dev/console case, since that runs with no filehandles. (But<br>
personally that leans me towards not erroring; Your <strike>Mileage</strike><br>
Scar Tissue May Vary.)<br>
<br>
> /me wonders whether i could at least replace the special case with a fallback<br>
> that uses open() with O_TMPFILE?<br>
<br>
Problem: open("/", O_RDWR|O_TMPFILE|O_EXCL) needs write access to "/" even if it<br>
isn't creating a dentry, because the new inode uses that filesystem as backing<br>
store to flush pages to on memory pressure. (And if you do write 100 megabytes<br>
to it it'll try to store it instead of discarding it. (O_TMPFILE is mmapable,<br>
even with O_EXCL I think?)) Expecting a writeable directory like /dev or /tmp to<br>
exist is the same problem as expecting /dev/null to exist: chroot doesn't<br>
guarantee any specific filesystem layout.<br>
<br>
When I need a dummy filehandle without a filesystem I call pipe() and close the<br>
other end. (Writes would error but that happens with shell pipelines all the<br>
time...)<br>
<br>
That said, the /dev/null approach would probably be fine if the open was<br>
deferred until it was actually shown to be needed.<br>
<br>
> > because the last thing the world needs is _multiple_ copies of all the i18n<br>
> > data. icu is the source of truth for that on Android. (we've had bugs in the<br>
> > past in this area that made us clean up our act.)<br>
> <br>
> It's the dlopen() that hurts. I'd ask why there's no libicu.a, but the r21d ndk<br>
> doesn't seem to have a libicu.so either. I should grab a newer version...<br>
> <br>
> icu4c in apps is ... complicated. but you're never getting a *static* icu4c for<br>
> the same "the icu data format and schema aren't stable" reasons i've mentioned<br>
> before.<br>
<br>
So utf8 won't ever work in static bionic the same way DNS queries won't work in<br>
static glibc (because that needs to dlopen libnss).<br></blockquote><div><br></div><div>fancy stuff like widths won't work, no.</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">
> Static linking is a legitimate use case. Android does not particularly treat it<br>
> as such, but I'm going to keep periodically tapping the sign.<br>
> <br>
> it's a legitimate use case _if you're prepared to carry everything around<br>
> yourself_. and that is supported --- feel free to build your own icu4c and ship<br>
> your own 30MiB data file :-)<br>
<br>
*shrug* Musl seems to do it well enough for me. I admit I'm not a polyglot.<br></blockquote><div><br></div><div>yeah, the libc APIs aren't really useful for any serious i18n work anyway (look at the amount of detail lost in the translation from the icu4c API to the wcwidth() one, for example).</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">
> (we did finally get a stable supported subset of the icu4c API for apps into the<br>
> NDK for S aka 12, but that doesn't help your use case.)<br>
<br>
I need to make the /system symlink you suggested work. Possibly in a mkroot<br>
image under qemu. Unfortunately it was fiddly enough I didn't get through it<br>
over the weekend and it went back on the todo list.<br>
<br>
> I was hoping that after the aosp build there'd be a system directory that I<br>
> could ln -s /system to and it would just work. Instead:<br>
> <br>
> # source this script in bash<br>
> <br>
> source ${ANDROID_BUILD_TOP}/build/envsetup.sh<br>
> <br>
> So we have to do the envsetup in order for $ANDROID_BUILD_TOP to be set, and<br>
> then this script does it _again_. Right.<br>
> <br>
> (fwiw, i think that's for the buildbots.)<br>
> <br>
> Before I run something that makes this many assumptions as root so it can make<br>
> presumably persistent changes to my host system, I think I'm going to try to<br>
> read it a little more closely. Thanks for the pointer.<br>
> <br>
> oh, yeah, to be clear: i was suggesting you boil this down to the three or four<br>
> lines you actually need (since aiui part of your goal is to _not_ have to have a<br>
> complete AOSP checkout that's up to date and built).<br>
<br>
I like having an up to date AOSP checkout, but updating and rebuilding is<br>
nontrivial for me.<br>
<br>
> > that should set you up with everything you need (and both 32- and 64-bit<br>
> too, if<br>
> > you're chasing something 32-bit-only).<br>
> <br>
> It declares a shell function, and then doesn't call it. What it DOES do is eval<br>
> a soong invocation, and says to read envsetup.sh (which is just under 2000 lines<br>
> long) to figure out what --dumpvars-mode does (that env and declare -p<br>
> presumably don't).<br>
><br>
> yeah, you just need the `ln -s`s.<br>
<br>
The ones under:<br>
<br>
if [ ${TARGET_ARCH} = x86 -o ${TARGET_ARCH} = x86_64 ]; then<br>
<br>
Which apparently don't have a corresponding arm version?<br></blockquote><div><br></div><div>yeah, i don't have an arm host that's faster than a phone anyway.</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">
> if you follow the history far back enough, it<br>
> used to basically just be that, but things got a lot more complicated to support<br>
> all the different kinds of builds, and the multiple supported build environments<br>
> (bots versus humans in particular).<br>
<br>
I know the feeling.<br>
<br>
I spent a chunk of the weekend staring at scripts/mkroot.sh trying to figure out<br>
how to simplify it. I'm unhappy with all the setup plumbing before "Create new<br>
root filesystem's directory layout", it's too complicated. (And I'm trying to<br>
add record-commands support to it, which doesn't help.)<br>
<br>
> if you just manually do the `ln -s` for /system/bin and /system/lib64 (and<br>
> /system/lib if you care about 32-bit), you're most of the way there, and i think<br>
> the error messages you get when you run will point you at the other bits (tz data?).<br>
<br>
Might take a stab this weekend, but right now I'm hip deep in yocto plumbing.<br>
(Which is the opposite of fun but pays the bills.)<br>
<br>
Rob<br>
</blockquote></div></div>