Re: [Buildroot] [PATCH 2/7] package/minijail: new package

["José Pekkarinen" <jose.pekkarinen@unikie.com>]

On Wed, Jan 12, 2022 at 4:51 PM Thomas Petazzoni
<thomas.petazzoni@bootlin.com> wrote:
>
> Hello,
>
> On Wed, 12 Jan 2022 16:15:16 +0200
> José Pekkarinen <jose.pekkarinen@unikie.com> wrote:
>
> > > What are you "fixing"? You're replacing a prlimit() call by by
> > > setrlimit(), but why? What problem does it solve? Why is it correct to
> > > do that?
> >
> >     The problem is shown from as this:
> >
> > /home/pekkari/br-test-pkg/bootlin-armv5-uclibc/build/minijail-linux-v17/libminijail.c:
> > In function ‘set_rlimits_or_die’:
> > /home/pekkari/br-test-pkg/bootlin-armv5-uclibc/build/minijail-linux-v17/libminijail.c:1911:7:
> > error: implicit declaration of function ‘prlimit’; did you mean
> > ‘setrlimit’? [-Werror=implicit-function-declaration]
> > 1911 |   if (prlimit(j->initpid, j->rlimits[i].type, &limit, NULL))
> >      |       ^~~~~~~
> >      |       setrlimit
> >
> >     So I followed the suggestion, no more, no less.
>
> You shouldn't blindly follow compiler suggestions :-)
>
> prlimit() exists in musl. Was <sys/resource.h> included, as suggested
> by the prlimit() man page ?

    It is:

$ cat libminijail.c
[...]
#include <sys/resource.h>
[...]

> > > > +-static_assert(FD_SETSIZE >= MAX_PRESERVED_FDS * 2 - 1,
> > > > +-          "If true, ensure_no_fd_conflict will always find an unused fd.");
> > >
> > > You're not fixing the static_assert() here but simply dropping it. Why?
> > > When does it fail? What is the problem with it? Why is it safe to drop
> > > it?
> >
> >     It fails like this:
> >
> > /home/pekkari/br-test-pkg/bootlin-armv5-uclibc/build/minijail-linux-v17/libminijail.c:2623:15:
> > error: expected declaration specifiers or ‘...’ before numeric
> > constant
> > 2623 | static_assert(FD_SETSIZE >= MAX_PRESERVED_FDS * 2 - 1,
> >      |               ^~~~~~~~~~
> > /home/pekkari/br-test-pkg/bootlin-armv5-uclibc/build/minijail-linux-v17/libminijail.c:2624:8:
> > error: expected declaration specifiers or ‘...’ before string constant
> > 2624 |        "If true, ensure_no_fd_conflict will always find an unused fd.");
> >      |        ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
> >
> >     So my understanding is that this platform may
> > define static_assert in a different manner, and I
> > find no traces of the purpose of that isolated assert,
> > so the easier way to get around this was removing it.
>
> Well, the assert is there for a reason, so we need to understand what
> it is verifying, and whether it is safe to drop it.
>
> After checking, FD_SETSIZE seems to be defined to 1024 by glibc, musl
> and uclibc-ng, and MAX_PRESERVED_FDS is defined as 128 by the minijail
> code.
>
> Can you try using _Static_assert() instead, which is part of the C11
> standard?

    I'll do.

    Thanks!

    José.
_______________________________________________
buildroot mailing list
buildroot@buildroot.org
https://lists.buildroot.org/mailman/listinfo/buildroot