[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <20240411015140.GZ4163@brightrain.aerifal.cx>
Date: Wed, 10 Apr 2024 21:51:40 -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 Wed, Apr 10, 2024 at 09:07:38PM -0400, Rich Felker wrote:
> 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?
Here's a draft of how initgroups could work. I dropped direct use of
mmap since all this code already depends on malloc, and using malloc
simplifies it.
This versions undefined NGROUPS_MAX and replaces it with 2 so that the
fallback path will get triggered to test it. That should be removed
(and probably macros to use internal malloc added) before it's merged.
This change is independent of any potential changes we make to the
macro or to sysconf.
Rich
View attachment "initgroups_draft.diff" of type "text/plain" (1189 bytes)
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.
