unistd: Add getgroups, setgroups, getgrouplist, initgroups #733

JayH5 · 2017-08-11T13:07:32Z

No description provided.

Susurrus · 2017-08-11T14:57:36Z

src/unistd.rs

@@ -942,6 +942,42 @@ pub fn setgid(gid: Gid) -> Result<()> {
 }

 #[inline]


Remove #[inline] for both functions. Also need documentation.

Thanks, was just copying without knowing what I was doing 😅. Docs added.

Susurrus · 2017-08-11T14:57:55Z

src/unistd.rs

@@ -942,6 +942,42 @@ pub fn setgid(gid: Gid) -> Result<()> {
 }

 #[inline]
+pub fn setgroups(groups: &[Gid]) -> Result<()> {
+    cfg_if! {
+        if #[cfg(any(target_os = "macos",


Alphabetize the target_os listing. This should be done everywhere.

asomers

What about tests? Perhaps you could add a test that round trips a gidset from setgroups through getgroups?

asomers · 2017-08-11T15:13:16Z

src/unistd.rs

+            type setgroups_ngroups_t = size_t;
+        }
+    }
+    let gids: Vec<gid_t> = groups.iter().cloned().map(Into::into).collect();


This looks like it does a data copy. Can you eliminate it? sys::aio::lio_listio does something similar.

I'm pretty new to Rust and had no idea you could do this. Not entirely comfortable doing so before there are tests, though.

asomers · 2017-08-11T15:13:44Z

src/unistd.rs

+    Errno::result(res).map(drop)
+}
+
+#[cfg(any(target_os = "linux", target_os = "android", target_os = "macos", target_os = "ios"))]


FreeBSD also has initgroups

Yes, but it's unfortunately not implemented in rust-lang/libc for FreeBSD yet.

asomers · 2017-08-11T17:05:27Z

src/unistd.rs

+        // from a group between the two calls and we don't want to incorrectly set the length of
+        // the Vec and expose uninitialized memory.
+        unsafe { groups.set_len(s as usize) };
+        groups.iter().cloned().map(|gid| Gid::from_raw(gid)).collect()


Can you avoid the data copy here? You may have to mem::transmute gid_ts into Gids.

I can sure give it a go. Like I said with the other case, I'd like to have tests first, if possible.

JayH5 · 2017-08-11T17:06:47Z

@asomers I don't know much about the test infrastructure, but setgroups() (and by extension, initgroups()) require root on most platforms. Is that possible?

I've added a getgroups() implementation for now, in case it is possible.

JayH5 · 2017-08-11T17:07:12Z

Thank you both for the reviews 😄

asomers · 2017-08-11T17:20:29Z

Rust's unit test framework lacks a skip method. So you should do a UID check. If non-zero, simply return 0.

Susurrus · 2017-08-12T18:08:46Z

src/unistd.rs

+/// of. The additional group `group` is also added to the list.
+///
+/// [Further reading](http://man7.org/linux/man-pages/man3/initgroups.3.html)
+// TODO: Get initgroups binding added for FreeBSD in libc


We should have this done before we merge this PR. Can you file a PR against libc adding the necessary FFI?

Created rust-lang/libc#726. Also added getgrouplist() as it'd help for testing initgroups(), at least on some platforms.

JayH5 · 2017-08-13T15:33:31Z

src/unistd.rs

+/// platforms. It returns the current group access list for the user associated
+/// with the effective user id of the process; the group access list may change
+/// over the lifetime of the process, and it is not affected by calls to
+/// `setgroups()`.


These notes are borrowed from the Python documentation: https://docs.python.org/3.6/library/os.html#os.getgroups

JayH5 · 2017-08-13T15:35:26Z

I can also implement getgrouplist(). I've done that here, but it is fairly complex and I'm not sure if it's OK to grow this PR any more.

Susurrus · 2017-08-13T15:48:01Z

I'm find with adding getgrouplist() into this PR since it's all closely related and the tests are different with it added.

Susurrus · 2017-08-13T15:49:01Z

I think we can also squash this into a single commit, or a commit per function rather than the 14 it currently stands at.

JayH5 · 2017-08-14T10:11:56Z

src/unistd.rs

+        Ok(None) | Err(_) => <c_int>::max_value(),
+    };
+    use std::cmp::min;
+    let mut ngroups = min(ngroups_max, 8);


8 is pretty arbitrarily chosen. NGROUPS_MAX can vary quite a bit:

macOS: 16

Debian (Linux/GNU): 65536

Alpine (Linux/musl): 32

What's the point of capping the number of groups?

Not sure I understand? I'm choosing 8 as a good size to start the groups buffer at. If groups is bigger than NGROUPS_MAX then the getgrouplist() call will error on most platforms.

Yeah, re-read the docs and it makes sense now.

JayH5 · 2017-08-14T10:13:48Z

I've been on leave before this week so I have less time now. My git rebasing skills are... a little rusty. Not sure when I'll get to cleaning up these commits but may only be on the weekend.

The code is all here for review for now at least.

Susurrus

It'd also be nice if there were some examples for using these functions? There are some examples in the man pages that might be worth copying. That being said I'm wondering if there isn't a better abstraction we can offer over a bare wrapper around the base functions? For example, I'm unclear when init_groups is supposed to be called, but if we can enforce that it's called at the right time through some higher level abstraction, I think that'd be good.

Susurrus · 2017-08-14T14:40:17Z

src/unistd.rs

+    Errno::result(res).map(drop)
+}
+
+/// Calculate the supplementary group access list. Gets the group IDs of all


Make the 2nd and third sentence a new paragraph.

Susurrus · 2017-08-14T14:40:51Z

src/unistd.rs

+/// groups that `user` is a member of. The additional group `group` is also
+/// added to the list.
+///
+/// *Note:* Although the `getgrouplist()` call does not return any specific


Put this under an # Errors section (see the book)

Susurrus · 2017-08-14T14:41:02Z

src/unistd.rs

+/// partial list of groups when `NGROUPS_MAX` is exceeded, this implementation
+/// will only ever return the complete list or else an error.
+///
+/// [Further reading](http://man7.org/linux/man-pages/man3/getgrouplist.3.html)


Put this above the errors section.

Susurrus · 2017-08-14T14:42:30Z

src/unistd.rs

+        Ok(None) | Err(_) => <c_int>::max_value(),
+    };
+    use std::cmp::min;
+    let mut ngroups = min(ngroups_max, 8);


What's the point of capping the number of groups?

Susurrus · 2017-08-14T14:43:13Z

src/unistd.rs

+        // Returns -1 if ngroups is too small, but does not set errno.
+        // BSD systems will still fill the groups buffer with as many groups
+        // as possible, but Linux manpages do not mention this behavior.
+        if ret == -1 {


This should be an else if to be more readable.

Susurrus · 2017-08-14T14:51:20Z

src/unistd.rs

+    let ret = unsafe { libc::getgroups(0, ptr::null_mut()) };
+    let mut size = try!(Errno::result(ret));
+
+    // Now actually get the groups


This comment should be expanded to explain why we need to loop here.

Susurrus · 2017-08-14T14:51:50Z

src/unistd.rs

+            Err(e) => return Err(e)
+        }
+    }
+    Ok(groups)


This should be replaced with an unreachable!.

Susurrus · 2017-08-14T14:52:28Z

src/unistd.rs

+        libc::setgroups(groups.len() as setgroups_ngroups_t, groups.as_ptr() as *const gid_t)
+    };
+
+    Errno::result(res).map(drop)


map(|_| ()) as well.

Susurrus · 2017-08-14T14:52:34Z

src/unistd.rs

+        }
+    }
+    // We can coerce a pointer to some `Gid`s as a pointer to some `gid_t`s as
+    // they have the same representation in memory.


Again, not actually true.

Susurrus · 2017-08-14T14:53:51Z

test/test_unistd.rs

@@ -104,6 +104,57 @@ mod linux_android {
    }
 }

+#[test]
+#[cfg(not(any(target_os = "ios", target_os = "macos")))]


Why aren't these run on these targets?

Because the getgroups() and setgroups() calls don't work as expected on Apple platforms, as discussed in the docs.

Please leave that as a comment in the code here for both this and the following test.

JayH5 · 2017-08-15T19:19:48Z

@Susurrus I guess the usual use for initgroup (or at least what I want to use it for) is when switching user to drop privileges. That usually works with passwd and group stuff that isn't in nix yet (and would probably be my next PR).

Hypothetically speaking, something like:

let user = "daemon";
let passwd = Passwd::for_name(&user);
initgroups(passwd.name, passwd.gid)?;
setgid(passwd.gid)?;
setuid(passwd.uid)?;

Guess I can try figure out an example without the passwd stuff though. Like, I said not a lot of free time right now but should get there...

Susurrus · 2017-08-25T21:55:31Z

That looks to be a reasonable example then, let's add it as one.

Susurrus · 2017-08-25T21:56:20Z

src/unistd.rs

+/// Set the list of supplementary group IDs for the calling process.
+///
+/// *Note:* On macOS, `getgroups()` may not return the same group list set by
+/// calling `setgroups()`. Apple discourages the use of `setgroups()`.


What does Apple suggest one use instead? It's frustrating when this is stated and then there's no suggest alternative.

Susurrus · 2017-08-25T21:57:44Z

test/test_unistd.rs

+    assert_eq!(new_groups, groups);
+
+    // Revert back to the old groups
+    setgroups(&old_groups).unwrap();


Shouldn't this have one last assertion following this to check that they were properly reverted?

The getgroups() and setgroups() calls at the start and end of these tests aren't really part of the tests, they're just a small attempt to restore the state of the current process to what it was before the test ran. 😕

Susurrus · 2017-08-25T21:58:30Z

test/test_unistd.rs

+#[cfg(not(any(target_os = "ios", target_os = "macos")))]
+fn test_initgroups() {
+    if !Uid::current().is_root() {
+        // initgroups(), setgroups() require root


Move this comment above the if and rephrase to something like "Skip this test when not run as root as initgroups() and setgroups() both require root."

Susurrus · 2017-08-25T21:58:43Z

test/test_unistd.rs

+    assert_eq!(new_groups, group_list);
+
+    // Revert back to the old groups
+    setgroups(&old_groups).unwrap();


Again, shouldn't there be one last assertion following this?

Susurrus · 2017-08-25T21:59:05Z

test/test_unistd.rs

@@ -104,6 +104,57 @@ mod linux_android {
    }
 }

+#[test]
+#[cfg(not(any(target_os = "ios", target_os = "macos")))]


Please leave that as a comment in the code here for both this and the following test.

Susurrus · 2017-08-26T16:40:30Z

Alright, can you squash everything into a single commit and add a CHANGELOG entry for this?

@asomers Can you approve your review if everything's been addressed?

Once that's all done we should be good to merge!

JayH5 · 2017-08-27T18:15:51Z

@Susurrus I've rebased to fewer, clearer commits. If you want just one commit then GitHub can do that when we merge?

Thanks for the reviewing 😄

Susurrus · 2017-08-27T21:41:04Z

The number of commits are fine now, you didn't need to segregate them all, but it's fine.

You have failures in CI that need resolving before we can merge.

Also, @asomers can you resolve your review here? I want to make sure everything's fine with you (please click the "Approve review" or whatever link down towards the bottom of the page).

JayH5 · 2017-08-28T07:27:42Z

Looks like rust-lang/libc#742 broke initgroups on Linux platforms. I can try fix, but may only get to it next weekend.

asomers · 2017-08-28T16:40:05Z

test/test_unistd.rs

+fn test_setgroups() {
+    // Skip this test when not run as root as `setgroups()` requires root.
+    if !Uid::current().is_root() {
+        return


Until Cargo adds a way to skip tests, you should print something here to let the user know that the test was skipped. Otherwise it will rot, because people won't realize that the test isn't run. See tests/test_mount.rs. Ditto for line 137.

asomers · 2017-08-28T16:44:21Z

test/test_unistd.rs

+#[test]
+// `getgroups()` and `setgroups()` do not behave as expected on Apple platforms
+#[cfg(not(any(target_os = "ios", target_os = "macos")))]
+fn test_setgroups() {


These two tests race against each other. You must add a mutex to prevent that. grep for CWD_MTX for an example.

Susurrus · 2017-10-07T14:53:47Z

I have a few comments I made a while ago that didn't get a response and should get one before I'm okay with this PR. I don't have any other comments after those are all resolved.

Susurrus · 2017-11-08T05:06:56Z

@JayH5 Pinging you on this as we'd love to get it merged.

JayH5 · 2017-11-11T18:04:09Z

@Susurrus sorry for the slow reply, I was on holiday.

I think I've responded to all your comments now, except:

I'd think it'd still be good to test the mac/ios functionality if we could. Do you understand how they work enough to write some tests for them, even if they don't cover all functionality? I don't like that we document how Apple's platforms are different and then just don't test them even though it's clear we should know how they do work well enough to test them.

I'm honestly not sure what to do about this. I tried adding a test for Apple platforms based on the following note in the getgroups manpage:

Calling initgroups(3) to opt-in for supplementary groups will cause getgroups() to return a single entry, the GID that was passed to initgroups(3)

But I don't actually see that behaviour on my machine running High Sierra (getgroups returns more than one entry after a call to initgroups).

So, honestly, no, I don't know enough about the way these calls work on Apple platforms to write tests for them. I could experiment and test what the behaviour is but I'm not sure I have the time/patience to do that. Perhaps these functions could just be disabled on Apple platforms?

Susurrus · 2017-11-11T18:02:25Z

src/unistd.rs

+                // resize of the groups Vec and try again...
+                let cap = groups.capacity();
+                unsafe { groups.set_len(cap) };
+                groups.reserve(1);


Yes, a comment specifying exactly that would be awesome.

Susurrus · 2017-11-11T18:16:14Z

And I agree with your final assessment above, that this should be disabled on Apple platforms since it's unclear what the expected behavior should be. Let's just make it clear in the documentation why this is disabled on Apple targets.

Fix groups mutex name

Don't need to call as_c_str()

JayH5 · 2017-11-12T11:08:52Z

[...]this should be disabled on Apple platforms since it's unclear what the expected behavior should be. Let's just make it clear in the documentation why this is disabled on Apple targets.

@Susurrus I've tried to do this now. Not sure if these functions should go inside a submodule, like with unistd::setres?

Susurrus · 2017-11-12T23:09:58Z

@JayH5 There's no need to put them in a submodule (I'd actually suggest we take those functions out of a submodule). But this LGTM. Thanks for your PR!

bors r+

733: unistd: Add getgroups, setgroups, getgrouplist, initgroups r=Susurrus a=JayH5

bors · 2017-11-12T23:35:13Z

Build succeeded

Susurrus suggested changes Aug 11, 2017

View reviewed changes

asomers requested changes Aug 11, 2017

View reviewed changes

asomers reviewed Aug 11, 2017

View reviewed changes

Susurrus approved these changes Aug 11, 2017

View reviewed changes

JayH5 changed the title ~~unistd: Add setgroups and initgroups~~ unistd: Add getgroups, setgroups and initgroups Aug 11, 2017

Susurrus suggested changes Aug 12, 2017

View reviewed changes

JayH5 commented Aug 13, 2017

View reviewed changes

JayH5 changed the title ~~unistd: Add getgroups, setgroups and initgroups~~ unistd: Add getgroups, setgroups, getgrouplist, initgroups Aug 14, 2017

JayH5 commented Aug 14, 2017

View reviewed changes

Susurrus suggested changes Aug 14, 2017

View reviewed changes

Susurrus suggested changes Aug 25, 2017

View reviewed changes

Susurrus approved these changes Aug 26, 2017

View reviewed changes

JayH5 force-pushed the groups-funcs branch from e6fb608 to e30da93 Compare August 27, 2017 18:13

JayH5 added a commit to JayH5/nix that referenced this pull request Aug 27, 2017

Add changelog entry for nix-rust#733

e30da93

asomers requested changes Aug 28, 2017

View reviewed changes

JayH5 force-pushed the groups-funcs branch from e30da93 to 9a92ad5 Compare September 9, 2017 19:19

JayH5 added a commit to JayH5/nix that referenced this pull request Sep 9, 2017

Add changelog entry for nix-rust#733

01fa5c7

JayH5 force-pushed the groups-funcs branch from 44bf23c to 7ba1f56 Compare November 11, 2017 16:16

JayH5 added a commit to JayH5/nix that referenced this pull request Nov 11, 2017

Add changelog entry for nix-rust#733

abf5002

JayH5 force-pushed the groups-funcs branch 2 times, most recently from 5b4f9e3 to d6f42a3 Compare November 11, 2017 17:31

Susurrus suggested changes Nov 11, 2017

View reviewed changes

JayH5 added 12 commits November 12, 2017 10:56

unistd: Remove trailing whitespace

7fff6d5

unistd: Add setgroups()

a154b26

unistd: Add getgroups()

5ff0c35

test_unistd: Add test for getgroups/setgroups()

89b7055

unistd: Add initgroups()

7263c5a

unistd: Add getgrouplist()

4a2d65e

test_unistd: Add test for getgrouplist/initgroups()

688667e

Add changelog entry for nix-rust#733

e36d4af

groups tests: Add groups mutex and print message when tests skipped

a7ebd89

Fix groups mutex name

unistd: Make getgrouplist and initgroups take &CStr

57246cf

Don't need to call as_c_str()

unistd: groups: Respond to minor PR feedback items

a75f3fa

unistd: groups: Disable functions on Apple platforms

d4e7761

JayH5 force-pushed the groups-funcs branch from d6f42a3 to d4e7761 Compare November 12, 2017 09:48

unistd: Rework import of std::ptr

0c786df

Susurrus approved these changes Nov 12, 2017

View reviewed changes

bors bot added a commit that referenced this pull request Nov 12, 2017

Merge #733

7d7e392

733: unistd: Add getgroups, setgroups, getgrouplist, initgroups r=Susurrus a=JayH5

bors bot merged commit 0c786df into nix-rust:master Nov 12, 2017

SteveLauC mentioned this pull request Dec 4, 2023

initgroups erroneously not defined on macOS #2069

Open

		@@ -942,6 +942,42 @@ pub fn setgid(gid: Gid) -> Result<()> {
		}

		#[inline]

unistd: Add getgroups, setgroups, getgrouplist, initgroups #733

unistd: Add getgroups, setgroups, getgrouplist, initgroups #733

Conversation

JayH5 commented Aug 11, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

asomers left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

JayH5 commented Aug 11, 2017

JayH5 commented Aug 11, 2017

asomers commented Aug 11, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

JayH5 commented Aug 13, 2017

Susurrus commented Aug 13, 2017

Susurrus commented Aug 13, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

JayH5 commented Aug 14, 2017

Susurrus left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

JayH5 commented Aug 15, 2017

Susurrus commented Aug 25, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Susurrus commented Aug 26, 2017

JayH5 commented Aug 27, 2017

Susurrus commented Aug 27, 2017

JayH5 commented Aug 28, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Susurrus commented Oct 7, 2017

Susurrus commented Nov 8, 2017

JayH5 commented Nov 11, 2017

Choose a reason for hiding this comment

Susurrus commented Nov 11, 2017

JayH5 commented Nov 12, 2017

Susurrus commented Nov 12, 2017

bors bot commented Nov 12, 2017

Build succeeded