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

[enh]

On Wed, Sep 1, 2021 at 3:00 PM Rob Landley <rob at landley.net> wrote:

> 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.
>

to me, if the open() succeeded but the write() failed --- we're in weird
uncharted "shouldn't happen" territory, and probably want to know?


> 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),


(i used to find that annoying, but since my team has to update the compiler
for Android, i do now see the flip side of it: that "true" -Wall would mean
you could never update the compiler. especially in a -Werror world. we have
enough work just dealing with the new warnings that _do_ get added to
-Wall! 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?".)


> 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.
>

i'd have assumed (as you can tell from my patch :-) ) that "you asked me to
set the selinux labels but i couldn't" == error, not warning?

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.
>

seems like a feature to me? don't say --selinux if you don't actually care?


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

computer says "yes":

#### build completed successfully (6 seconds) ####


> 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...
> _______________________________________________
> Toybox mailing list
> Toybox at lists.landley.net
> http://lists.landley.net/listinfo.cgi/toybox-landley.net
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.landley.net/pipermail/toybox-landley.net/attachments/20210902/cb663a31/attachment-0001.htm>