-
Notifications
You must be signed in to change notification settings - Fork 675
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
Add UnixCredentials support on FreeBSD/DragonFly #1216
Conversation
d0a32db
to
ba031dd
Compare
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.
This looks pretty good! One thing I would suggest would be to consider creating separate types for Linux vs the BSDs. Nix's purpose is to provide a Rusty API to system libraries, not to provide a cross-platform API. If separate types with different APIs solve the invalid uid problem, then it would be worthwhile.
src/sys/socket/mod.rs
Outdated
@@ -498,6 +561,11 @@ impl ControlMessageOwned { | |||
let cred: libc::ucred = ptr::read_unaligned(p as *const _); | |||
ControlMessageOwned::ScmCredentials(cred.into()) | |||
} | |||
#[cfg(any(target_os = "freebsd", target_os = "dragonfly"))] | |||
(libc::SOL_SOCKET, /* libc::SCM_CREDS */ 0x03) => { |
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.
Adding a note to this line so we don't forget to fix it before committing.
src/sys/socket/mod.rs
Outdated
@@ -729,6 +819,8 @@ impl<'a> ControlMessage<'a> { | |||
ControlMessage::ScmRights(_) => libc::SCM_RIGHTS, | |||
#[cfg(any(target_os = "android", target_os = "linux"))] | |||
ControlMessage::ScmCredentials(_) => libc::SCM_CREDENTIALS, | |||
#[cfg(any(target_os = "freebsd", target_os = "dragonfly"))] | |||
ControlMessage::ScmCredentials(_) => /* libc::SCM_CREDS */ 0x03, // https://github.com/rust-lang/libc/pull/1740 |
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.
Adding a comment here to ensure we fix this before merging
2322289
to
c5accbb
Compare
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.
Much improved. I like the new constructorless ControlMessage
. Ping me after the libc PR merges and then we can finish this one.
#[cfg(any(target_os = "freebsd", target_os = "dragonfly"))] | ||
ControlMessage::ScmCreds => { | ||
// The kernel overwrites the data, don't worry | ||
return |
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.
Could this result in cmsg_data
being uninitialized? That way might lead to UB. I think that you should zero it instead.
The libc PR (rust-lang/libc#1740) has apparently been merged, if that may be of use to this one. |
ping @myfreeweb . It looks like the libc PR has been merged. Can you finish this one now? |
c5accbb
to
b2a9c4f
Compare
It's still not released to crates.io.. Okay, switched to git for now. |
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.
bors r+
Build succeeded: |
This allows working with
SCM_CREDS
messages, which are likeSCM_CREDENTIALS
on Linux, but slightly different (always overwritten by the kernel, contain a bit more info — euid and groups).With this PR, it is possible to write portable code that would use the appropriate message for the platform, but one remaining quirk is that
PassCred
thing still has to be present andcfg
'd to Linux.Adding the
SCM_CREDS
constant to libc: rust-lang/libc#1740