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

Jump to bottom

expose getpwent and getgrent #1820

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

expose getpwent and getgrent #1820

wants to merge 1 commit into from

Conversation

SteveLauC
Copy link
Member

This PR is for issue #1811.

On OSes where getpwent_r/getgrent_r is available, it is used. On other OSes, use getpwent/getgrent. However, illumos and solaris, they do have getpwent_r/getgrent_r, but the definitions exposed by libc are wrong, so I currently use getpwent/getgrent on them.

The implementations of AllUsers::next and AllGroups::next on platforms where getpwent_r/getgrent_r is available are kind of ugly, this is because the return values of these functions are so different:

  • freebsd: getpwent_r man page

    1. On success: return 0, result will be non-NULL
    2. On error: return the error number, result will be NULL
    3. End: return 0, result will be NULL.

  • dragonfly: getpwent_r man page

    1. On success: return 0 and result will be non-NULL
    2. On error: return the error number, result will be NULL
    3. End: return 0, result will be NULL

The return values on freebsd and dragonfly are the same.

  • netbsd: getpwent_r man page

    1. On success: return 0, result will be non-NULL,
    2. On error: return a non-zero value (errno is set), result will be NULL
    3. End: return 0, result will be NULL

  • Linux: getpwent_r man page

    1. On success: return 0, result will be non-NULL
    2. On error: return the error number (not ENOENT), result will be NULL
    3. End: return ENOENT and result will be NULL.

Another thing I am not sure about is the return value of AllUsers::next()/AllGroups::next(). In my implementation, it is:

fn next(&mut self) -> Option<Result<User/Group, Errno>>

The inner Result is used to expose the errno value, the drawback of such a design is that the user has to stop iterating when an error occurs.

@SteveLauC
Copy link
Member Author

Haven't added the changes to CHANGELOG.md.

Will add it, maybe after #1817 is merged, or there will be a merge conflict.

@SteveLauC
Copy link
Member Author

SteveLauC commented Sep 16, 2022 •
edited

The CI for fuchsia failed because setgrent/getgrent/endgrent are currently not exposed in libc.

PR filed.

asomers
asomers reviewed Sep 17, 2022
View reviewed changes
Copy link
Member

@asomers asomers left a comment

Choose a reason for hiding this comment

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

This setpwent business sucks. Very un-Rusty. I wonder if it would better just to use the raceless fgetpwent_r function instead. I think that's only available on GNU/Linux, but at least it's thread-safe.

src/unistd.rs
///
/// # Safety
///
/// This function is marked as `unsafe` cause `setpwent()` and `endpwent()`
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
/// This function is marked as `unsafe` cause `setpwent()` and `endpwent()`
/// This function is marked as `unsafe` because `setpwent()` and `endpwent()`

src/unistd.rs

#[cfg(any(
target_os = "freebsd",
target_os = "netbsd",
Copy link
Member

Choose a reason for hiding this comment

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

What about OpenBSD?

Copy link
Member Author

@SteveLauC SteveLauC Sep 18, 2022 •
edited

Choose a reason for hiding this comment

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

OpenBSD does not have getpwent_r and getgrent_r so I am using getpwent/getgrent on it.

src/unistd.rs
/// # Safety
///
/// This function is marked as `unsafe` cause `setpwent()` and `endpwent()`
/// will modify a global state, which will result in a data race if we have
Copy link
Member

Choose a reason for hiding this comment

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

What kind of data race? Skipping over some users, or duplicating some, is not considered unsafe. Only something like reading a struct with some entries from one source and some from another would be considered unsafe.

Copy link
Member Author

Choose a reason for hiding this comment

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

There are two issues here I think:

  1. The pointer returned by getpwent() points to static memory, and thus can be overwritten by subsequent calls like getpwent, getpwuid, and getpwnam. So in the implementation using getpwent(), when executing Some(Ok(User::from(unsafe{&*pwd}))) in a thread (say thread A), there happens to be another thread (thread B) calling getpwent/getpwuid/getpwnam, then the static memory will be overwritten due to the call in thread B, making thread A get a wrong result. This is the data race.
  2. setpwent/endpwent will modify a global state, if two instances are running at the same time, the state set by the first instance can be reset by the second one, then the first instance will get duplicate entries.

Sorry for the unclarity in my documentation, will update it stating both issues.

src/unistd.rs
/// ```
// RedoxFS does not support passwd and android does not have /etc/passwd.
#[cfg(not(any(target_os = "redox", target_os = "android")))]
pub unsafe fn all_users() -> AllUsers {
Copy link
Member

Choose a reason for hiding this comment

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

I suggest that a more natural API would be to call this function AllUsers::new()

src/unistd.rs
// no more entries
if res.is_null() {
None
}else {
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
}else {
} else {

@SteveLauC
Copy link
Member Author

This setpwent business sucks. Very un-Rusty. I wonder if it would better just to use the raceless fgetpwent_r function instead. I think that's only available on GNU/Linux, but at least it's thread-safe.

Yes, this API sucks. What are we going to do about this pr? Change the implementation for GNU/Linux to fgetpwent_r, what about the other platforms?

@SteveLauC SteveLauC mentioned this pull request Sep 25, 2022
Merged
bors bot added a commit that referenced this pull request Sep 27, 2022
@bors @SteveLauC
Merge #1828
5db70b6 
1828: bump libc to 0.2.133 r=rtzoeller a=SteveLauC

Bump `libc` so that `setgrent/getgrent/endgrent` are available on fuchsia, and thus the CI failure in #1820 will be resolved

Co-authored-by: Steve Lau <stevelauc@outlook.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@asomers asomers asomers left review comments

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
2 participants
@SteveLauC @asomers