bpo-35070: test_getgrouplist may fail on macOS if too many groups#13071
Conversation
The test_getgroups subtest fails when the current user running the test belongs to the maximum number supplemental groups. It fails because posix.getgrouplist() passes an array to the POSIX API function getgrouplist() that is too small. It uses NGROUPS_MAX to determine the size of the array that it passes to getgrouplist() to be filled with the user's GIDs. NGROUPS_MAX is defined by POSIX.1 as the maximum number of supplimental groups a user can belong to. The value has to be increased by one because getgrouplist() returns both the supplemental groups and the primary group, i.e. all of the groups the user belongs to. NGROUPS_MAX is defined as 16 on OS X systems, so the test fails when the user belongs to 16 supplemental groups because it passes an array with 16 elements to receive the GIDs, but getgrouplist() returns an error because it wants to return 17 GIDs (the primary plus 16 supplemntals). This problem most likely is not seen on Linux systems because they typically have a much higher limit for supplemental groups (e.g. 65,536 on Ubuntu 16.04.4).
|
Hello @websurfer5. Try edit the PR's title and change |
websurfer5
changed the title
dpage
left a comment
dpage
left a comment
There was a problem hiding this comment.
This patch resolves the regression test failures I've been seeing on macOS Mojave.
| @@ -0,0 +1 @@ | |||
| posix.getgrouplist() fails when when the user has NGROUPS_MAX supplemental GIDs. This becomes a problem on OS X Mojave (14.x) because the system defines NGROUPS_MAX to be 16 and it is common for a user to belong to 16 groups. The fix is to increase the size of the array passed to the underlying getgrouplist() POSIX API function to allow space for the user's primary group, which is not accounted for in the NGROUPS_MAX constant (per POSIX.1). No newline at end of file | |||
There was a problem hiding this comment.
Thanks for adding the blurb entry. It's a bit too verbose, though. It should only be one or two sentences and just needs to say something about preventing test_getgrouplist failures on macOS. If anyone needs to find more detaill, they can refer to the code. Also, this problem is not new to macOS 10.14; it only depends on how many groups the user is a member of. So the test may pass on 10.14 and may fail on older systems.
| * and the primary group, i.e. all of the groups the | ||
| * user belongs to. | ||
| */ | ||
| #define MAX_GROUPS (NGROUPS_MAX + 1) |
There was a problem hiding this comment.
MAX_GROUPS is also defined later in posixmodule.c in os_getgroups_impl. Without also changing it, the fix here can cause a compile warning:
../../source/Modules/posixmodule.c:6831:9: warning: 'MAX_GROUPS' macro redefined [-Wmacro-redefined]
#define MAX_GROUPS NGROUPS_MAX
^
../../source/Modules/posixmodule.c:6746:9: note: previous definition is here
#define MAX_GROUPS (NGROUPS_MAX + 1)
^
The simplest fix is to also change the latter definition to be NGROUPS_MAX + 1.
There was a problem hiding this comment.
I have made the requested changes; please review again
|
A Python core developer has requested some changes be made to your pull request before we can consider merging it. If you could please address their requests along with any other requests in other reviews from core developers that would be appreciated. Once you have made the requested changes, please leave a comment on this pull request containing the phrase And if you don't make the requested changes, you will be put in the comfy chair! |
ned-deily
changed the title
…istent throughout the source file; update news blurb with requested changes
|
Thanks @websurfer5 for the PR, and @ned-deily for merging it 🌮🎉.. I'm working now to backport this PR to: 3.7. |
…thonGH-13071) (cherry picked from commit 8725c83) Co-authored-by: Jeffrey Kintscher <49998481+websurfer5@users.noreply.github.com>
|
GH-14042 is a backport of this pull request to the 3.7 branch. |
|
Thanks @websurfer5 for the PR, and @ned-deily for merging it 🌮🎉.. I'm working now to backport this PR to: 3.7. |
|
Thanks @websurfer5 for the PR, and @ned-deily for merging it 🌮🎉.. I'm working now to backport this PR to: 3.8. |
…thonGH-13071) (cherry picked from commit 8725c83) Co-authored-by: Jeffrey Kintscher <49998481+websurfer5@users.noreply.github.com>
|
GH-14043 is a backport of this pull request to the 3.8 branch. |
7 participants
The test_getgroups subtest fails when the current user running the
test belongs to the maximum number supplemental groups. It
fails because posix.getgrouplist() passes an array to the POSIX API
function getgrouplist() that is too small. It uses NGROUPS_MAX to
determine the size of the array that it passes to getgrouplist()
to be filled with the user's GIDs. NGROUPS_MAX is defined by POSIX.1
as the maximum number of supplimental groups a user can belong to.
The value has to be increased by one because getgrouplist() returns
both the supplemental groups and the primary group, i.e. all of the
groups the user belongs to.
NGROUPS_MAX is defined as 16 on OS X systems, so the test fails when
the user belongs to 16 supplemental groups because it passes an array
with 16 elements to receive the GIDs, but getgrouplist() returns an
error because it wants to return 17 GIDs (the primary plus 16
supplemntals).
This problem most likely is not seen on Linux systems because they typically
have a much higher limit for supplemental groups (e.g. 65,536 on
Ubuntu 16.04.4).
https://bugs.python.org/issue35070