-
Notifications
You must be signed in to change notification settings - Fork 673
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
unistd: Add getgroups, setgroups, getgrouplist, initgroups #733
Changes from all commits
7fff6d5
a154b26
5ff0c35
89b7055
7263c5a
4a2d65e
688667e
e36d4af
a7ebd89
57246cf
a75f3fa
d4e7761
0c786df
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -4,7 +4,7 @@ use nix::unistd::*; | |
use nix::unistd::ForkResult::*; | ||
use nix::sys::wait::*; | ||
use nix::sys::stat; | ||
use std::{env, iter}; | ||
use std::{self, env, iter}; | ||
use std::ffi::CString; | ||
use std::fs::File; | ||
use std::io::Write; | ||
|
@@ -122,6 +122,73 @@ mod linux_android { | |
} | ||
} | ||
|
||
#[test] | ||
// `getgroups()` and `setgroups()` do not behave as expected on Apple platforms | ||
#[cfg(not(any(target_os = "ios", target_os = "macos")))] | ||
fn test_setgroups() { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. These two tests race against each other. You must add a mutex to prevent that. grep for |
||
// Skip this test when not run as root as `setgroups()` requires root. | ||
if !Uid::current().is_root() { | ||
let stderr = std::io::stderr(); | ||
let mut handle = stderr.lock(); | ||
writeln!(handle, "test_setgroups requires root privileges. Skipping test.").unwrap(); | ||
return; | ||
} | ||
|
||
#[allow(unused_variables)] | ||
let m = ::GROUPS_MTX.lock().expect("Mutex got poisoned by another test"); | ||
|
||
// Save the existing groups | ||
let old_groups = getgroups().unwrap(); | ||
|
||
// Set some new made up groups | ||
let groups = [Gid::from_raw(123), Gid::from_raw(456)]; | ||
setgroups(&groups).unwrap(); | ||
|
||
let new_groups = getgroups().unwrap(); | ||
assert_eq!(new_groups, groups); | ||
|
||
// Revert back to the old groups | ||
setgroups(&old_groups).unwrap(); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Shouldn't this have one last assertion following this to check that they were properly reverted? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The |
||
} | ||
|
||
#[test] | ||
// `getgroups()` and `setgroups()` do not behave as expected on Apple platforms | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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. |
||
#[cfg(not(any(target_os = "ios", target_os = "macos")))] | ||
fn test_initgroups() { | ||
// Skip this test when not run as root as `initgroups()` and `setgroups()` | ||
// require root. | ||
if !Uid::current().is_root() { | ||
let stderr = std::io::stderr(); | ||
let mut handle = stderr.lock(); | ||
writeln!(handle, "test_initgroups requires root privileges. Skipping test.").unwrap(); | ||
return; | ||
} | ||
|
||
#[allow(unused_variables)] | ||
let m = ::GROUPS_MTX.lock().expect("Mutex got poisoned by another test"); | ||
|
||
// Save the existing groups | ||
let old_groups = getgroups().unwrap(); | ||
|
||
// It doesn't matter if the root user is not called "root" or if a user | ||
// called "root" doesn't exist. We are just checking that the extra, | ||
// made-up group, `123`, is set. | ||
// FIXME: Test the other half of initgroups' functionality: whether the | ||
// groups that the user belongs to are also set. | ||
let user = CString::new("root").unwrap(); | ||
let group = Gid::from_raw(123); | ||
let group_list = getgrouplist(&user, group).unwrap(); | ||
assert!(group_list.contains(&group)); | ||
|
||
initgroups(&user, group).unwrap(); | ||
|
||
let new_groups = getgroups().unwrap(); | ||
assert_eq!(new_groups, group_list); | ||
|
||
// Revert back to the old groups | ||
setgroups(&old_groups).unwrap(); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Again, shouldn't there be one last assertion following this? |
||
} | ||
|
||
macro_rules! execve_test_factory( | ||
($test_name:ident, $syscall:ident, $exe: expr) => ( | ||
#[test] | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
8 is pretty arbitrarily chosen.
NGROUPS_MAX
can vary quite a bit:There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What's the point of capping the number of groups?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not sure I understand? I'm choosing 8 as a good size to start the
groups
buffer at. Ifgroups
is bigger thanNGROUPS_MAX
then thegetgrouplist()
call will error on most platforms.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, re-read the docs and it makes sense now.