[Toybox] [PATCH] Specify owner when testing tar with /dev/null

[enh]

On Thu, Dec 7, 2023 at 1:39 PM Rob Landley <rob at landley.net> wrote:
>
>
>
> On 12/7/23 11:46, enh wrote:
> > On Thu, Dec 7, 2023 at 9:45 AM Colin Cross <ccross at google.com> wrote:
> >>
> >> On Thu, Dec 7, 2023 at 8:26 AM enh <enh at google.com> wrote:
> >> >
> >> > On Wed, Dec 6, 2023 at 6:44 PM Rob Landley <rob at landley.net> wrote:
> >> > >
> >> > > On 12/6/23 18:15, Colin Cross wrote:
> >> > > >> Who _does_ own /dev/null in your test environment?
> >> > > >
> >> > > > "nobody":
> >> > > >
> >> > > >     FAIL: tar pass /dev/null
> >> > > >     echo -ne '' | tar c --mtime @0 --group sys /dev/null 2>/dev/null | LST
> >> > > >     --- expected 2023-12-07 00:03:55.797797713 +0000
> >> > > >     +++ actual 2023-12-07 00:03:55.805797762 +0000
> >> > > >     @@ -1 +1 @@
> >> > > >     -crw-rw-rw- root/sys 1,3 1970-01-01 00:00 dev/null
> >> > > >     +crw-rw-rw- nobody/sys 1,3 1970-01-01 00:00 dev/null
> >> > > >     external/toybox/toybox-gtests.cpp:168: Failure
> >> > > >     Expected equality of these values:
> >> > > >       exit_status
> >> > > >         Which is: 1
> >> > > >       0
> >> > >
> >> > > Does bionic still have getuid() stubbed out? (I.E. does toybox's "stat -c %U/%G
> >> > > /dev/null" work?)
> >> >
> >> > you mean getpwuid(), i assume? but i don't think that's relevant,
> >> > because i think ccross is talking about host testing. that's in an
> >> > nsjail set up by build/soong/ui/build/sandbox_linux.go.
> >>
> >> This isn't the Soong nsjail sandbox, this is a sandbox created by some
> >> combination of a python script (atest), which calls a java tool
> >> (tradefed), which runs bazel, which runs the test.  My guess is that
> >> the sandboxing is all happening in bazel's test runner.
> >
> > ah, is that sandbox relevant now Android's not moving to bazel then?
> > or does the soong sandbox have the same problem with this test?
>
> My concern was more "if I check that toybox stat and toybox tar come up with the
> same string for the owner of /dev/null, will toybox stat linked against bionic
> produce a useful answer?" But I think I answered my own question: if stat can't
> then tar already couldn't.
>
> Commit 4225f1a9d015 already broke this where the comments say things like "fetch
> uid, specify gid" but the code specifies both and fetching is not tested at all.
> And the proposed commit here would have again "passed" tests by removing what
> they test.
>
> Try commit eccdfdf8e7ef, which fetches the names to match with stat.
>
> Rob
>
> P.S. My lib/passwd.c rewrite is hardwiring in /etc/passwd and /etc/gshadow and
> so on, in the classic colon separated field format, so commands don't have to
> #include <shadow.h> and can build more of defconfig against the NDK. But I'm not
> sure what format bionic is currently fetching that data out of (it doesn't seem
> to come up much), and don't want switching that on to break the test here...?
> Does there need to be lib/portability.c shenanigans to fix up my fixup?

yeah, there's no file for us[1]. the toys that edit /etc/passwd should
just be disabled for Android because they make no sense. the query
functions will want to defer to bionic to get the right results on
Android.

(given that there's no modifiable user database on Android, i'm still
not sure why you'd _want_ to be able to build more of defconfig
against the NDK. i think there's a similar situation with macOS where
some of the "trivial wrapper around a linux syscall" toys aren't going
to be usefully amenable to a portability.c fix, and just want a
"REQUIRES linux" type thing in the config.)

____
1. it's a bit more complicated than that, but "true enough" for the
purposes of your specific question.