🐛(chg): don't crash when user has more than 16 groups on macOS #573

vegerot · 2023-03-20T17:54:05Z

🐛(chg): don't crash when user has more than 16 groups on macOS

Summary:
D43676809 (OSS: 49ee17f) introduced a check for whether the chg server and
sl process have different permissions. We run getgroups in two different
places:

using os.getgroups in sl client in Python
using libc's unistd/getgroups in chg server in C

Essentially the way this works is we call os.getgroups in the client (Python)
and unistd/getgroups on the chg server (C). Then, the server compares the
results. If they're the same, then both processes have the same permissions,
otherwise they might be different.

However, getgroups on macOS has a key limitation that it will only return a
max of NGROUPS_MAX groups (16 groups on my machine, defined in
sys/param.h). You can remove this limit by defining
_DARWIN_UNLIMITED_GETGROUPS or _DARWIN_C_SOURCE¹.

os.getgroups in Python is compiled with _DARWIN_C_SOURCE² but chg does
not, meaning that the result of getgroups is different in our Python and C
code.

This commit defines _DARWIN_UNLIMITED_GETGROUPS for chg so getgroups will
have consistent behavior. (the alternative is patching CPython to unset
_DARWIN_C_SOURCE which is worse)

Special thanks to @strager for helping me debug this 👍🏻

Test plan:

have user with <=16 groups
run sl --version
confirm it works
have user with >16 groups
run sl --version
notice sl failing with error "chg: abort: too many redirections."
apply this patch
repeat steps 4-5
confirm it works

Co-authored-by: strager Strager.nds@gmail.com

vegerot · 2023-03-20T18:26:56Z

I would like to add a test to prevent future regressions but don't know an easy way to write a reliable test for this. test-fb-ext-grpcheck just mocks os.getgroups but I'd need an actual user to confirm the difference _DARWIN_UNLIMITED_GETGROUPS makes

strager · 2023-03-20T20:13:07Z

eden/scm/setup.py

@@ -1669,6 +1669,12 @@ def cythonize(*args, **kwargs):
                "depends": [versionhashpath],
                "include_dirs": ["contrib/chg"] + include_dirs,
                "extra_args": filter(None, cflags + chgcflags + [STDC99, WALL, PIC]),
+                # chg uses libc::unistd/getgroups() to check that chg and the
+                # sl cli have the same permissions (see 49ee17f2bfad).


Nit: I'd write D43676809 instead of the open source Git commit hash (49ee17f2bfad).

off-topic: sapling doesn't support referencing commits by Revision:

❯ sl show D43676809 abort: unknown revision 'D43676809'!

so I need to sl show the git commit and find the revision#.
that'd be a neat feature to add, but it'd only make sense in open source FB repos

Made a similar change to the commit message too

it'd only make sense in open source FB repos

LLVM too!

strager · 2023-03-20T20:17:57Z

eden/scm/setup.py

+                # however, on macOS, getgroups() is limited to NGROUPS_MAX (16) groups by default.
+                # we can work around this by defining _DARWIN_UNLIMITED_GETGROUPS
+                # see https://opensource.apple.com/source/xnu/xnu-3247.1.106/bsd/man/man2/getgroups.2.auto.html
+                "macros": [("_DARWIN_UNLIMITED_GETGROUPS", "1")]


Should we also write a comment in the calls to getgroups? I'm thinking about the situation where a programmer ports the chg client (or server) code to Rust. They'll see a call to getgroups and just call libc::getgroups without checking that it matches the other half. This would have a chance of reintroducing the bug (because there are no tests to detect it).

In my codebases, I would write # NOTE[macos-getgroups]: ... here, and in the getgroups calls I would write # See NOTE[macos-getgroups]. to link to this note. I don't know what the convention is for the Sapling codebase.

Good idea. I would rather just write a test, but having to create a user and add groups to it in the sl test environment sounds very tricky to do without polluting the user's machine

Honestly the commit message goes much more in-depth than even this comment. Should I put the commit message in some docs somewhere and reference it here and with the getgroups calls?

@strager

Summary: D43676809 (OSS: 49ee17f) introduced a check for whether the chg server and sl process have different permissions. We run `getgroups` in two different places: 1. using `os.getgroups` in sl client in Python 2. using libc's `unistd/getgroups` in chg server in C Essentially the way this works is we call `os.getgroups` in the client (Python) and `unistd/getgroups` on the chg server (C). Then, the server compares the results. If they're the same, then both processes have the same permissions, otherwise they might be different. However, `getgroups` on macOS has a key limitation that it will only return a max of `NGROUPS_MAX` groups (16 groups on my machine, defined in `sys/param.h`). You can remove this limit by defining `_DARWIN_UNLIMITED_GETGROUPS` or `_DARWIN_C_SOURCE`[^1]. `os.getgroups` in Python is compiled with `_DARWIN_C_SOURCE`[^2] but chg does not, meaning that the result of `getgroups` is different in our Python and C code. This commit defines `_DARWIN_UNLIMITED_GETGROUPS` for chg so `getgroups` will have consistent behavior. (the alternative is patching CPython to unset `_DARWIN_C_SOURCE` which is worse) Special thanks to @strager for helping me debug this 👍🏻 Test plan: 1. have user with <=16 groups 2. run `sl --version` 3. confirm it works 4. have user with >16 groups 5. run `sl --version` 6. notice sl failing with error "chg: abort: too many redirections." 7. apply this patch 8. repeat steps 4-5 9. confirm it works [^1]: https://opensource.apple.com/source/xnu/xnu-3247.1.106/bsd/man/man2/getgroups.2.auto.html [^2]: https://github.com/python/cpython/blob/28d369e070652e8f2274101d72131e3140dfadf7/configure.ac#L267 Co-authored-by: strager <Strager.nds@gmail.com>

facebook-github-bot · 2023-03-24T18:33:40Z

@quark-zju has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator.

facebook-github-bot · 2023-03-28T23:34:49Z

@quark-zju merged this pull request in 5656ac6.

facebook-github-bot added the CLA Signed label Mar 20, 2023

vegerot force-pushed the pr573 branch 2 times, most recently from 1d83bc7 to 327cd92 Compare March 20, 2023 20:11

strager reviewed Mar 20, 2023

View reviewed changes

vegerot force-pushed the pr573 branch from 327cd92 to b2c7423 Compare March 20, 2023 21:50

vegerot force-pushed the pr573 branch from b2c7423 to 49d5c4f Compare March 20, 2023 22:08

quark-zju mentioned this pull request Mar 28, 2023

ImportIt does not preserve Unicode correctly facebookarchive/fbshipit#156

Closed

facebook-github-bot closed this in 5656ac6 Mar 28, 2023

facebook-github-bot added the Merged label Mar 28, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

🐛(chg): don't crash when user has more than 16 groups on macOS #573

🐛(chg): don't crash when user has more than 16 groups on macOS #573

vegerot commented Mar 20, 2023 •

edited

vegerot commented Mar 20, 2023 •

edited

strager Mar 20, 2023

vegerot Mar 20, 2023 •

edited

vegerot Mar 20, 2023

strager Mar 20, 2023

strager Mar 20, 2023

vegerot Mar 20, 2023

vegerot Mar 22, 2023

facebook-github-bot commented Mar 24, 2023

facebook-github-bot commented Mar 28, 2023

🐛(chg): don't crash when user has more than 16 groups on macOS #573

🐛(chg): don't crash when user has more than 16 groups on macOS #573

Conversation

vegerot commented Mar 20, 2023 • edited

Footnotes

vegerot commented Mar 20, 2023 • edited

strager Mar 20, 2023

Choose a reason for hiding this comment

vegerot Mar 20, 2023 • edited

Choose a reason for hiding this comment

vegerot Mar 20, 2023

Choose a reason for hiding this comment

strager Mar 20, 2023

Choose a reason for hiding this comment

strager Mar 20, 2023

Choose a reason for hiding this comment

vegerot Mar 20, 2023

Choose a reason for hiding this comment

vegerot Mar 22, 2023

Choose a reason for hiding this comment

facebook-github-bot commented Mar 24, 2023

facebook-github-bot commented Mar 28, 2023

vegerot commented Mar 20, 2023 •

edited

vegerot commented Mar 20, 2023 •

edited

vegerot Mar 20, 2023 •

edited