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

[Rob Landley]

On 8/31/21 11:16 AM, enh via Toybox wrote:
>   toys/posix/tar.c:821:7: error: ignoring return value of
>   function declared with 'warn_unused_result' attribute
>       write(sefd, 0, 0);
>       ^~~~~ ~~~~~~~~~~
> ---
>  toys/posix/tar.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)

Yeah, I thought about that when I wrote it, but it seems like the correct thing
to do on failure here _is_ nothing? We already warned if the open() or the
write() of the xattr data failed, this is just telling it to reset after the
initial write() to set it displayed any error and set exitval.

I thought about typecasting it to (void) but decided to wait for a complaint
because A)
"CROSS_COMPILE=~/android/android-ndk-r21d/toolchains/llvm/prebuilt/linux-x86_64/bin/x86_64-linux-android-
make distclean defconfig toybox" didn't warn about it (obviously -Wall doesn't
mean all, that would be silly), B) that meant I couldn't be sure the typecast is
still allowed to squelch it. (Compiler writers are notoriously crotchety about
simple ways to stop them from insisting they know better than the person writing
the program. Well, the gcc guys were.)

I didn't use xwrite() here because do we really want the program to exit? I
dunno what the potential failure modes here are because "man setfscreatecon"
says on error it returns -1 and DOES NOT MENTION errno. (Bravo. The more I read
of libselinux the less actual engineering competence seems to have gone into it.)

But I know that _my_ code:

            i = strlen(pp);
            sefd = xopen("/proc/self/attr/fscreate", O_WRONLY|WARN_ONLY);
            if (sefd==-1 || i!=write(sefd, pp, i))
              perror_msg("setfscreatecon %s", pp);

Didn't close sefd and set it back to -1, it let the exit path to do that. (Since
it was already going to.) Meaning the initial write would warn but not exit, and
then this would exit trying to reset what we never had permission to set,
instead of continuing on to extract what it can.

Although the tar code can survive fd = -1 (when it has selinux data to deploy
it'll warn and move on, when the tarball hasn't got an selinux entry for the
file specifying --selinux is a NOP) it presumably won't ever get it because
"/proc/self/attr/fscreate" seems to always be there (since commit 1b556412becc8
in 2003, which went in circa 2.5.60), even when selinux isn't enabled. I'm not
sure under which circumstances trying to read from or write to it produces
errors because fs/proc/base.c has this horrible LSM_DIR_OPS() macro that's
trying to be template instantiation in C (I have yet to find any PART of selinux
implementation that isn't disgusting) and I lost interest in reading further.

But on my devuan system without selinux enabled:

  $ cat /proc/self/attr/fscreate
  cat: /proc/self/attr/fscreate: Invalid argument

So I _think_ your patch changes "tar x --selinux" behavior to exit instead of
spamming warnings when run on a system without sexlinux support.

Does the (void)write(...); typecast still shut up the warning I can't reproduce?

Rob

P.S. gcc has somehow managed to get worse. A dynamically linked bionic toybox says:

$ ldd ./toybox
./toybox: error while loading shared libraries:
/usr/lib/x86_64-linux-gnu/libm.so: invalid ELF header

Because /usr/lib/x86_64-linux-gnu/libm.so is a linker script. Bra-filesystem
checking-vo. But hey, it built without warnings which is what I was trying to
check. An in the absence of the string "bionic" in it, there's at least
"note.android" to confirm the binary came from the android NDK toolchain. Or
just V=1 so I can see the invocations go by with the right toolchain path...