[Toybox] [PATCH] tar: fix warn_unused_result error.

[Rob Landley]

On 9/2/21 7:12 PM, enh wrote:
> On Thu, Sep 2, 2021 at 4:37 PM Rob Landley <rob at landley.net
>     > to me, if the open() succeeded but the write() failed --- we're in weird
>     > uncharted "shouldn't happen" territory, and probably want to know?
> 
>     Hmmm, I thought the open would succeed and the write fail, but if it's reliable
>     that the open is what fails when selinux is disabled...
> 
> (note that i haven't tested that!)

And I can't because I don't have selinux test environment coverage, or the
stomach to read through enough of the terrible selinux code in the kernel.

This is not an area I have sufficient existing expertise to feel confident
about, and unlike sed/bzip/bash/awk/bc I'm not planning on doing a multi-month
deep dive to BUILD said expertise. (Because I still think NSA selinux was a
terrible idea and cannot generate the necessary enthusiasm.)

>     There's your problem: never -Werror. 
> 
> if you don't -Werror, you inevitably check in code with warnings, spamming the
> build and making it easier for more warnings to creep in.

And then you remove the warnings again? Toybox doesn't have -Werror...

Way back when the linux kernel was full of warnings because they got lost in the
noise of the always verbose output, and I made a blueberry.py script that would
filter the output into something intelligible
(https://static.lwn.net/2002/0117/bigpage.php3#:~:text=Blueberry), and the
kernel guys did their usual response to a "don't ask questions post errors" and
implemented the functionality in a much less terrible way inside (what became)
kbuild, and once you could SEE the warnings they removed them from the build.

I'm not sure "the linux kernel is so much smaller and simpler than our package,
it just doesn't have the scalability problems we do" is a compelling argument
for most packages?

>     Compiler version updates always hallucinate
>     new weirdness. If nothing else you're making it harder to build old versions in
>     new build environments for regression testing comparisons.
> 
> with hermetic builds you're using the right compiler for that version anyway.
> (forget features: when 100M lines of code you have, reliant on compiler bugs you
> will be.)

As with "reproducible builds", that's true if you accept it as a premise and a
lot of tedious work to fix when you don't. :P

This is why I wanted to split the build into layers:

1) Base OS layer with libc+toybox plus native compiler (and maybe kernel):
minimum needed to rebuild itself under itself in a tiny VM/container accessed
via serial console or similar. (Which makes it self-regression-testing to an
extent.)

2) Natively building the generic non-gui infrastructure layer: git/repo if it
isn't in the base, probably ninja and cmake go here, more languages (perl,
python, java, lua..), more libraries (ssl, ncurses, libxml, libusb...), better
access mechanism (ssh or adb), whatever daemons define your environment (httpd,
bind, cups... I know android has its own suite of these starting with a custom
init and a security  management thingy but don't have a relationship diagram in
front of me...)

3) Building the GUI/desktop can of worms, UNDER stage 2 above.

Back under aboriginal linux I had layer 1 working pretty well and had a proof of
concept linux from scratch layer 2, and a friend built a gentoo layer 2 but it
was hack city. I did a couple years of research on building a red hat layer 2, a
debian layer 2, and a suse layer 2, but all four of those distros were
incestuous tangled crap. (Debian was the least bad, which is not an endorsement.)

One of the advantages of a layered approach is you can stop and snapshot. Build
stage 2 on top of a known stage 1, build stage 3 on top of a known stage 2.

Shipping your own base OS layer (to build stage 2 on top of a known stage 1)
_is_ a hermetic build. Rebuilding stage 3 on top of a known stage 2 should be
similarly orthogonal. (Or at least versionable.)

And I still think that at some point the NDK build should be dogfooded as the
AOSP toolchain build. (Maybe with a config switch or extra build stage
overlaying libraries NDK doesn't ship. But then "adding libraries after the fact
to an existing filesystem" is a normal part of the stage 2 build above? :)

I don't have my head properly wrapped around how selinux annotation works into
this, but I do know there are tools to annotate a filesystem after the fact
because I keep bumping into references to them. And I suspect there are more
granular approaches but splitting off those 3 layers seems like the obvious start...

>     > and any
>     > given warning's false positive rate can be highly dependent on the specific
>     > codebase, and i don't get the feeling compiler writers think about scaling to
>     > "can i rebuild all of Debian?" or even just "can i rebuild all of Android?".)
> 
>     Their userbase kinda does.
> 
> it's too late by then :-(

Alas, they're using the Microsoft approach: your users are unpaid beta testers,
you ship perpetually broken crap and course correct on the fly as the bug
reports come in, which never quite keeps up with new breakage.

>     Those are all "cannot continue to parse the input file format", not "output had
>     a problem"...
> 
> ah, so you're talking about the perror_msg() versus perror_exit() distinction.
> still a _failure_ but not an immediate one. (though as someone who always uses
> 'v' with tar, i'd never notice the failure interactively unless it was in the
> last 40 lines!)

Hence the exit code so at least your script notices. Also, my debian host tar says:

  tar: Exiting with failure status due to previous errors

Which is a sadly verbose message but we should say _something_. Hmmm...

>     >     So I _think_ your patch changes "tar x --selinux" behavior to exit
>     instead of
>     >     spamming warnings when run on a system without sexlinux support.
>     >
>     > seems like a feature to me? don't say --selinux if you don't actually care?
> 
>     Which would mean --selinux is special and behaves differently from the rest
>     of tar.
> 
> i was going to say "but if you got the wrong selinux labels, what you wrote out
> probably won't work" ... but then realized that's true of chmod() too. i'm not
> going to be able to run that binary if its 'x' bit isn't set, any more than if
> it doesn't have the right selinux label.

I just added a "tar: had errors" message at the end, ala "oy, check the backscroll".

Rob