Date: Fri, 02 Jul 2021 23:58:26 -0300 From: Érico Nogueira <ericonr@...root.org> To: <musl@...ts.openwall.com> Subject: NGROUPS_MAX definition conflicts with kernel one Hello all. I brought this up on IRC and we thought it best to bring it up on the ML as well. musl's NGROUPS_MAX is defined to 32, which is the kernel's definition for the value for linux < 2.6.4. Meanwhile, linux >= 2.6.4 has 65536. Linux 2.6.4 also introduced /proc/sys/kernel/ngroups_max, to allow for this value to be changed in the kernel without libc having to keep changing their definitions. glibc's headers define NGROUPS_MAX to 65536, the new limit, so there is already some discrepancy between the two libraries. Fortunately, the man pages for getgroups(2) suggests using sysconf(_SC_NGROUPS_MAX) instead of the NGROUPS_MAX value directly, since the sysconf function can, theoretically, query the information from the currently running kernel. glibc's does just that: it tries to read from /proc, and if that fails, it will return NGROUPS_MAX as defined in <limits.h>. Unfortunately, musl's implementation directly returns NGROUPS_MAX from its own definition of it, which means it returns 32. There are few scenarios where I can see this being relevant, to be honest; hardly anyone is going to have more than 32 supplementary groups. However, in the case they do, functions like getgroups(2) will fail even if the caller passes a buffer with size sysconf(_SC_NGROUPS_MAX). An application could be forgiven for not trying to increase the buffer size past it in the case that sysconf(_SC_NGROUPS_MAX) exists (doesn't return -1). There are a few ways forward from here: NGROUPS_MAX can be updated to the latest kernel maximum and/or sysconf(_SC_NGROUPS_MAX) can be fixed to try and read values from /proc; or we do nothing. Not doing nothing would also require rolling out further fixes: - musl's initgroups(3) uses a static array with 32 members for the groups, which isn't enough if we want to support cases with more groups; that would require implementing dynamic allocation, which, with naive logic, would allocate 256KB (65536 entries * sizeof(gid_t)) - any applications/libraries mainly used in the musl ecosystem might be relying on NPROCS_MAX being a small value that can be used, for example, as the size of a stack-allocated array; with a bigger size, this would no longer be true (see  as an example, though I am already looking into fixing that particular case)  https://github.com/pikhq/musl-nscd/blob/9ab730cfb5fe3dc224ded1033e1b36ea60c60be6/src/socket_handle.c#L232 In conclusion, as mentioned on IRC, "it's a place where the old limit was a feature", since it avoids big allocations when very very few use cases need them. IMO, if we want to be more correct while keeping the advantage of a low value, sysconf(_SC_NPROCS_MAX) could read /proc, but NPROCS_MAX could be kept as 32. But I don't know how the initgroups(3) case would deal with it. If we decide to do nothing, such a limitation (even if derived from a design choice) should at least end up documented clearly. A suggestion on IRC was also that users requiring more than 32 supplementary groups could patch musl to work on their environment. For that purpose, I have attached a patch to this email to make it simpler for such patching to occur. Cheers, Érico View attachment "0001-use-NGROUPS_MAX-instead-of-hardcoded-value-where-pos.patch" of type "text/plain" (1351 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.