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

[Thomas Petazzoni <thomas.petazzoni@bootlin.com>]

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 ?


> > > +-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?

Best regards,

Thomas
-- 
Thomas Petazzoni, co-owner and CEO, Bootlin
Embedded Linux and Kernel engineering and training
https://bootlin.com
_______________________________________________
buildroot mailing list
buildroot@buildroot.org
https://lists.buildroot.org/mailman/listinfo/buildroot