Skip to content
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.

Sign up for GitHub

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

Add getpwnam and related functions #864

Closed
wants to merge 17 commits into from
Closed

Add getpwnam and related functions #864

Show file tree
Hide file tree
Changes from 4 commits
Commits
Show all changes
17 commits
Select commit Hold shift + click to select a range
0846f15
Add `Users` and `Group` related functions
otavio Aug 19, 2019
072c08d
Initial rework of code; later will be squashed
otavio Aug 20, 2019
16f3cf5
Reduce initialization
otavio Aug 20, 2019
a8143fb
Use MaybeUninit
otavio Aug 21, 2019
de1ed63
Avoid using From<*mut libc::...> using reference
otavio Aug 22, 2019
51d39cf
Reduce code duplication
otavio Aug 22, 2019
3b75439
Handle ERANGE
otavio Aug 22, 2019
b3d7716
Use Result<Option<User|Group>> so it is more ergonimic
otavio Aug 22, 2019
b361db0
Improve `from_anything` definition using where clause
otavio Aug 28, 2019
76e33fe
Add a generic reserve_buffer_size method
otavio Sep 4, 2019
5eda92f
Rework generic reserve_buffer_size code
otavio Sep 5, 2019
d0b23f3
Use reserve_double_buffer_size<T> as method name
otavio Sep 5, 2019
56b0cd1
Fix unused code warnings
otavio Sep 5, 2019
cc9f81c
Drop Default from User as this should be inquired
otavio Sep 11, 2019
65ba0e5
Use 16384 as limit
otavio Sep 11, 2019
9111d12
Rework more reservation code to use reserve_double_buffer_size
otavio Sep 11, 2019
3fe2c01
Rework getgroups() according to @asomers suggestions
otavio Sep 21, 2019
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
248 changes: 128 additions & 120 deletions src/unistd.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2435,8 +2435,8 @@ pub struct User {
pub expire: libc::time_t
}

impl From<*mut libc::passwd> for User {
fn from(pw: *mut libc::passwd) -> User {
impl From<&libc::passwd> for User {
fn from(pw: &libc::passwd) -> User {
unsafe {
User {
name: CStr::from_ptr((*pw).pw_name).to_string_lossy().into_owned(),
Expand All @@ -2459,6 +2459,45 @@ impl From<*mut libc::passwd> for User {
}

impl User {
fn from_anything(f: impl Fn(*mut libc::passwd,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This signature is so big that it would probably be written better with a where clause:

    fn from_anything<F>(f: F) -> Result<Option<Self>>
    where
        F: Fn(
            *mut libc::passwd,
            *mut libc::c_char,
            libc::size_t,
            *mut *mut libc::passwd,
        ) -> libc::c_int

*mut libc::c_char,
libc::size_t,
*mut *mut libc::passwd) -> libc::c_int)
-> Result<Option<Self>>
{
let bufsize = match sysconf(SysconfVar::GETPW_R_SIZE_MAX) {
Ok(Some(n)) => n as usize,
Ok(None) | Err(_) => 1024 as usize,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not that it's defined in any publicly visible place, but FreeBSD internally sets this limit to 1MB.

};

let mut cbuf = Vec::with_capacity(bufsize);
let mut pwd = mem::MaybeUninit::<libc::passwd>::uninit();
let mut res = ptr::null_mut();

loop {
let error = unsafe {
Errno::clear();
f(pwd.as_mut_ptr(), cbuf.as_mut_ptr(), cbuf.capacity(), &mut res)
};

if error == 0 {
if res.is_null() {
return Ok(None);
} else {
let pwd = unsafe { pwd.assume_init() };
return Ok(Some(User::from(&pwd)));
}
} else if Errno::last() == Errno::ERANGE {
// Trigger the internal buffer resizing logic of `Vec` by requiring
// more space than the current capacity.
unsafe { cbuf.set_len(cbuf.capacity()); }
cbuf.reserve(1);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Extending by only one byte at a time could take awhile. It would be better to double the storage instead. That's what the C library does. Also, you shouldn't extend the buffer ad infinitum. At some point you should give up and return ERANGE. The C library does that at 1MB.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we can add a top limit, however, the same strategy is used inside std. For example: https://doc.rust-lang.org/src/alloc/vec.rs.html#949

Also, inside nix code it is also used. See: https://github.com/nix-rust/nix/blob/master/src/unistd.rs#L579-L583

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The link from inside of Vec is very different. In that case, it really is only 1 additional element's worth of space that is needed. Here, an unknown amount of additional space is needed. That's why I suggest doubling the storage. Nix's getcwd() method should probably do the same.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The link from inside of Vec is very different. In that case, it really is only 1 additional element's worth of space that is needed. Here, an unknown amount of additional space is needed. That's why I suggest doubling the storage. Nix's getcwd() method should probably do the same.

From reading the std code, it seems different. See:

https://github.com/rust-lang/rust/blob/bdfd698f37184da42254a03ed466ab1f90e6fb6c/src/liballoc/raw_vec.rs#L494-L500

and

https://github.com/rust-lang/rust/blob/bdfd698f37184da42254a03ed466ab1f90e6fb6c/src/liballoc/raw_vec.rs#L421-L434

So it seems todo exactly this; it seems good to keep the code simple but if really desired, I can rework this.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That's just the current implementation. Nix shouldn't rely upon it. We should actually try to reserve a reasonable amount of space for what our application needs.

} else {
return Err(Error::Sys(Errno::last()));
}
}
}

/// Get a user by UID.
///
/// Internally, this function calls
Expand All @@ -2468,35 +2507,14 @@ impl User {
///
/// ```
/// use nix::unistd::{Uid, User};
/// // Returns an Option<Result<User>>, thus the double unwrap.
/// let res = User::from_uid(Uid::from_raw(0), Some(2048)).unwrap().unwrap();
/// // Returns an Result<Option<User>>, thus the double unwrap.
/// let res = User::from_uid(Uid::from_raw(0)).unwrap().unwrap();
/// assert!(res.name == "root");
/// ```
pub fn from_uid(uid: Uid, bufsize: Option<usize>) -> Option<Result<Self>> {
let mut cbuf = Vec::with_capacity(bufsize.unwrap_or(PWGRP_BUFSIZE));
let mut pwd = mem::MaybeUninit::<libc::passwd>::uninit();
let mut res = ptr::null_mut();

let error = unsafe {
Errno::clear();
libc::getpwuid_r(
uid.0,
pwd.as_mut_ptr(),
cbuf.as_mut_ptr(),
cbuf.capacity(),
&mut res
)
};

if error == 0 {
if ! res.is_null() {
Some(Ok(User::from(res)))
} else {
None
}
} else {
Some(Err(Error::Sys(Errno::last())))
}
pub fn from_uid(uid: Uid) -> Result<Option<Self>> {
User::from_anything(|pwd, cbuf, cap, res| {
unsafe { libc::getpwuid_r(uid.0, pwd, cbuf, cap, res) }
})
}

/// Get a user by name.
Expand All @@ -2508,35 +2526,15 @@ impl User {
///
/// ```
/// use nix::unistd::User;
/// // Returns an Option<Result<User>>, thus the double unwrap.
/// let res = User::from_name("root", Some(2048)).unwrap().unwrap();
/// // Returns an Result<Option<User>>, thus the double unwrap.
/// let res = User::from_name("root").unwrap().unwrap();
/// assert!(res.name == "root");
/// ```
pub fn from_name(name: &str, bufsize: Option<usize>) -> Option<Result<Self>> {
let mut cbuf = Vec::with_capacity(bufsize.unwrap_or(PWGRP_BUFSIZE));
let mut pwd = mem::MaybeUninit::<libc::passwd>::uninit();
let mut res = ptr::null_mut();

let error = unsafe {
Errno::clear();
libc::getpwnam_r(
CString::new(name).unwrap().as_ptr(),
pwd.as_mut_ptr(),
cbuf.as_mut_ptr(),
cbuf.capacity(),
&mut res
)
};

if error == 0 {
if ! res.is_null() {
Some(Ok(User::from(res)))
} else {
None
}
} else {
Some(Err(Error::Sys(Errno::last())))
}
pub fn from_name(name: &str) -> Result<Option<Self>> {
let name = CString::new(name).unwrap();
User::from_anything(|pwd, cbuf, cap, res| {
unsafe { libc::getpwnam_r(name.as_ptr(), pwd, cbuf, cap, res) }
})
}
}

Expand All @@ -2551,8 +2549,8 @@ pub struct Group {
pub mem: Vec<String>
}

impl From<*mut libc::group> for Group {
fn from(gr: *mut libc::group) -> Group {
impl From<&libc::group> for Group {
fn from(gr: &libc::group) -> Group {
unsafe {
Group {
name: CStr::from_ptr((*gr).gr_name).to_string_lossy().into_owned(),
Expand Down Expand Up @@ -2580,6 +2578,45 @@ impl Group {
ret
}

fn from_anything(f: impl Fn(*mut libc::group,
*mut libc::c_char,
libc::size_t,
*mut *mut libc::group) -> libc::c_int)
-> Result<Option<Self>>
{
let bufsize = match sysconf(SysconfVar::GETGR_R_SIZE_MAX) {
Ok(Some(n)) => n as usize,
Ok(None) | Err(_) => 1024 as usize,
};

let mut cbuf = Vec::with_capacity(bufsize);
let mut grp = mem::MaybeUninit::<libc::group>::uninit();
let mut res = ptr::null_mut();

loop {
let error = unsafe {
Errno::clear();
f(grp.as_mut_ptr(), cbuf.as_mut_ptr(), cbuf.capacity(), &mut res)
};

if error == 0 {
if res.is_null() {
return Ok(None);
} else {
let grp = unsafe { grp.assume_init() };
return Ok(Some(Group::from(&grp)));
}
} else if Errno::last() == Errno::ERANGE {
// Trigger the internal buffer resizing logic of `Vec` by requiring
// more space than the current capacity.
unsafe { cbuf.set_len(cbuf.capacity()); }
cbuf.reserve(1);
} else {
return Err(Error::Sys(Errno::last()));
}
}
}

/// Get a group by GID.
///
/// Internally, this function calls
Expand All @@ -2591,35 +2628,14 @@ impl Group {
#[cfg_attr(not(target_os = "linux"), doc = " ```no_run")]
#[cfg_attr(target_os = "linux", doc = " ```")]
/// use nix::unistd::{Gid, Group};
/// // Returns an Option<Result<Group>>, thus the double unwrap.
/// let res = Group::from_gid(Gid::from_raw(0), Some(2048)).unwrap().unwrap();
/// // Returns an Result<Option<Group>>, thus the double unwrap.
/// let res = Group::from_gid(Gid::from_raw(0)).unwrap().unwrap();
/// assert!(res.name == "root");
/// ```
pub fn from_gid(gid: Gid, bufsize: Option<usize>) -> Option<Result<Self>> {
let mut cbuf = Vec::with_capacity(bufsize.unwrap_or(PWGRP_BUFSIZE));
let mut grp = mem::MaybeUninit::<libc::group>::uninit();
let mut res = ptr::null_mut();

let error = unsafe {
Errno::clear();
libc::getgrgid_r(
gid.0,
grp.as_mut_ptr(),
cbuf.as_mut_ptr(),
cbuf.capacity(),
&mut res
)
};

if error == 0 {
if !res.is_null() {
Some(Ok(Group::from(res)))
} else {
None
}
} else {
Some(Err(Error::Sys(Errno::last())))
}
pub fn from_gid(gid: Gid) -> Result<Option<Self>> {
Group::from_anything(|grp, cbuf, cap, res| {
unsafe { libc::getgrgid_r(gid.0, grp, cbuf, cap, res) }
})
}

/// Get a group by name.
Expand All @@ -2633,35 +2649,15 @@ impl Group {
#[cfg_attr(not(target_os = "linux"), doc = " ```no_run")]
#[cfg_attr(target_os = "linux", doc = " ```")]
/// use nix::unistd::Group;
/// // Returns an Option<Result<Group>>, thus the double unwrap.
/// let res = Group::from_name("root", Some(2048)).unwrap().unwrap();
/// // Returns an Result<Option<Group>>, thus the double unwrap.
/// let res = Group::from_name("root").unwrap().unwrap();
/// assert!(res.name == "root");
/// ```
pub fn from_name(name: &str, bufsize: Option<usize>) -> Option<Result<Self>> {
let mut cbuf = Vec::with_capacity(bufsize.unwrap_or(PWGRP_BUFSIZE));
let mut grp = mem::MaybeUninit::<libc::group>::uninit();
let mut res = ptr::null_mut();

let error = unsafe {
Errno::clear();
libc::getgrnam_r(
CString::new(name).unwrap().as_ptr(),
grp.as_mut_ptr(),
cbuf.as_mut_ptr(),
cbuf.capacity(),
&mut res
)
};

if error == 0 {
if !res.is_null() {
Some(Ok(Group::from(res)))
} else {
None
}
} else {
Some(Err(Error::Sys(Errno::last())))
}
pub fn from_name(name: &str) -> Result<Option<Self>> {
let name = CString::new(name).unwrap();
Group::from_anything(|grp, cbuf, cap, res| {
unsafe { libc::getgrnam_r(name.as_ptr(), grp, cbuf, cap, res) }
})
}
}

Expand Down Expand Up @@ -2719,18 +2715,24 @@ mod usergroupiter {
impl Iterator for Users {
type Item = Result<User>;
fn next(&mut self) -> Option<Result<User>> {

let mut cbuf = vec![0 as c_char; self.0];
let mut pwd: libc::passwd = unsafe { mem::uninitialized() };
let mut pwd = mem::MaybeUninit::<libc::passwd>::uninit();
let mut res = ptr::null_mut();

let error = unsafe {
Errno::clear();
libc::getpwent_r(&mut pwd, cbuf.as_mut_ptr(), self.0, &mut res)
libc::getpwent_r(
pwd.as_mut_ptr(),
cbuf.as_mut_ptr(),
self.0,
&mut res
)
};

let pwd = unsafe { pwd.assume_init() };

if error == 0 && !res.is_null() {
Some(Ok(User::from(res)))
Some(Ok(User::from(&pwd)))
} else if error == libc::ERANGE {
Some(Err(Error::Sys(Errno::last())))
} else {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think you can discount the possibility of other errors. POSIX lists EIO, EMFILE, and ENFILE, for example. You should return any error you receive.

Expand Down Expand Up @@ -2787,18 +2789,24 @@ mod usergroupiter {
impl Iterator for Groups {
type Item = Result<Group>;
fn next(&mut self) -> Option<Result<Group>> {

let mut cbuf = vec![0 as c_char; self.0];
let mut grp: libc::group = unsafe { mem::uninitialized() };
let mut grp = mem::MaybeUninit::<libc::group>::uninit();
let mut res = ptr::null_mut();

let error = unsafe {
Errno::clear();
libc::getgrent_r(&mut grp, cbuf.as_mut_ptr(), self.0, &mut res)
libc::getgrent_r(
grp.as_mut_ptr(),
cbuf.as_mut_ptr(),
self.0,
&mut res
)
};

let grp = unsafe { grp.assume_init() };

if error == 0 && !res.is_null() {
Some(Ok(Group::from(res)))
Some(Ok(Group::from(&grp)))
} else if error == libc::ERANGE {
Some(Err(Error::Sys(Errno::last())))
} else {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As above, don't swallow unexpected errnos.

Expand Down
12 changes: 0 additions & 12 deletions test/test_unistd.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -600,18 +600,6 @@ fn test_symlinkat() {
);
}

#[test]
fn test_getpwuid() {
let res = User::from_uid(Uid::from_raw(0), None).unwrap();
assert!(res.unwrap().uid == Uid::from_raw(0));
}

#[test]
fn test_getgrgid() {
let res = Group::from_gid(Gid::from_raw(0), None).unwrap();
assert!(res.unwrap().gid == Gid::from_raw(0));
}

#[cfg(not(any(target_os = "android",
target_os = "ios",
target_os = "macos",
Expand Down