[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Date: Wed, 10 Apr 2024 21:07:38 -0400
From: Rich Felker <dalias@...c.org>
To: Kate Deplaix <kit-ty-kate@...look.com>
Cc: "musl@...ts.openwall.com" <musl@...ts.openwall.com>
Subject: Re: [PATCH] Increase NGROUPS_MAX from 32 to 1024
On Tue, Nov 14, 2023 at 11:35:14PM +0000, Kate Deplaix wrote:
> Such a restrictive value for NGROUPS_MAX makes it impossible to have
> a musl-based system with a user belonging to more than 32 groups. If
> done on the root user, this will break your system.
> It also makes it impossible to use certain functions in binaries
> that have been compiled with musl.
>
> This new value is still very far from Linux's NGROUPS_MAX of 65536
> that has been there since Linux 2.6.4 but this is at least one tiny
> step in the right direction while maintainers investigate how to
> match Linux's value.
>
> ref: https://www.openwall.com/lists/musl/2021/07/03/1
> ref: https://www.openwall.com/lists/musl/2022/12/06/3
> ref: https://github.com/ocaml/opam/pull/5383
> ---
> include/limits.h | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/include/limits.h b/include/limits.h
> index 53a27b9d..501c3612 100644
> --- a/include/limits.h
> +++ b/include/limits.h
> @@ -45,7 +45,7 @@
> #define NAME_MAX 255
> #endif
> #define PATH_MAX 4096
> -#define NGROUPS_MAX 32
> +#define NGROUPS_MAX 1024
> #define ARG_MAX 131072
> #define IOV_MAX 1024
> #define SYMLOOP_MAX 40
> --
> 2.40.1
I've gone and read the POSIX text on NGROUPS_MAX again:
Runtime Increasable Values
The magnitude limitations in the following list shall be fixed by
specific implementations. An application should assume that the
value of the symbolic constant defined by <limits.h> in a specific
implementation is the minimum that pertains whenever the
application is run under that implementation. A specific instance
of a specific implementation may increase the value relative to
that supplied by <limits.h> for that implementation. The actual
value supported by a specific instance shall be provided by the
sysconf() function.
...
{NGROUPS_MAX}
Maximum number of simultaneous supplementary group IDs per
process.
and for getgroups:
If the effective group ID of the process is returned with the
supplementary group IDs, the value returned shall always be
greater than or equal to one and less than or equal to the value
of {NGROUPS_MAX}+1.
For the latter, this notation usually means not necessarily the macro
value, but the sysconf value obtainable at runtime.
Since there is no POSIX interface setgroups (this functionality is
left to the implementation), NGROUPS_MAX does not impose a requirement
to support setting that many groups. So increasing the value would not
make things "wrong on old kernels".
However POSIX also allows/encourages the macro to represent a "minimum
that pertains", which could reasonably be the 32, as long as
sysconf(_SG_NGROUPS_MAX) returns a value >= one less than the max
value getgroups can return at runtime.
I was worried using the procfs interface to make
sysconf(_SG_NGROUPS_MAX) dynamic may not be correct, as it could be
decreased after a process comes to have more supplemental groups than
the old limit. However, it appears this is just a read-only sysctl.
The ngroups_max variable was made const in commit
f628867da46f8867e1854e43d7200e42ec22eee2, but even before that the
file was read-only and did not seem to have any mechanism for changing
the limit. So it seems usable.
As for the macro, I think it's actually valid to define it as 65536,
since even if we're running on an old kernel, there is no conformance
distinction. I'm not sure if this is the nicest thing to do though.
Apps may want to start with a buffer of size NGROUPS_MAX and increase
it up to the sysconf value rather than allocating a giant amount of
memory that will never in practice be used. This should be further
discussed, particularly what impact it might have on application
behavior and memory usage.
Of course this still leaves us with what to do for initgroups. My
leaning is to have it behave as now, using a small stack buffer,
regardless of what we do with the macro, but with a change in the
error condition: when getgrouplist returns -1 with *ngroups greater
than the value passed-in, allocate a buffer this size with mmap (to
avoid pulling in a malloc dep; in the worst case it's large enough
malloc would use mmap anyway) and retry.
This avoids ever having to look at the kernel's value of ngroups_max;
setgroups will just fail (and initgroups will report the error) if
it's too large. It does have a TOCTOU race if the groups db changes
between the first call and the retry. If we care about that it could
loop on retry, and always increase the buffer size by at least 50%
each time so that it can't get stuck or consume more than a
logarithmic number of retries.
Thoughts? Does this sound reasonable? What to do with the macro?
Rich
Powered by blists - more mailing lists
Confused about mailing lists and their use?
Read about mailing lists on Wikipedia
and check out these
guidelines on proper formatting of your messages.
