Repository navigation
getuid, getgid: return host IDs when isolation is disabled - #5402
programskillforverification wants to merge 2 commits into
Conversation
There was a problem hiding this comment.
Thanks for giving this a shot!
However, it seems like this requires direct calls to libc to be implemented? That would then explain why we didn't do this before. Miri should never call libc directly. If there is no way to get the UID from a std API then I am afraid I don't think we should land this. If this is something that Rust programs commonly need, maybe you can convince t-libs to add it to the standard library. If not, it's not something we want to maintain on the Miri side. I understand that this is a somewhat arbitrary line, but we have to draw the line somewhere to avoid Miri scope-creeping into an unmaintainable mess of way too many complicated shims.
| fn getuid(&mut self) -> InterpResult<'tcx, Scalar> { | ||
| let this = self.eval_context_mut(); | ||
| this.assert_target_os_is_unix("getuid"); | ||
|
|
||
| interp_ok(Scalar::from_u32(this.get_uid(false))) | ||
| } | ||
|
|
||
| fn geteuid(&mut self) -> InterpResult<'tcx, Scalar> { | ||
| let this = self.eval_context_mut(); | ||
| this.assert_target_os_is_unix("geteuid"); | ||
|
|
||
| interp_ok(Scalar::from_u32(this.get_uid(true))) | ||
| } | ||
|
|
||
| fn getgid(&mut self) -> InterpResult<'tcx, Scalar> { | ||
| let this = self.eval_context_mut(); | ||
| this.assert_target_os_is_unix("getgid"); | ||
|
|
||
| interp_ok(Scalar::from_u32(this.get_gid(false))) | ||
| } | ||
|
|
||
| fn getegid(&mut self) -> InterpResult<'tcx, Scalar> { | ||
| let this = self.eval_context_mut(); | ||
| this.assert_target_os_is_unix("getegid"); | ||
|
|
||
| interp_ok(Scalar::from_u32(this.get_gid(true))) | ||
| } |
There was a problem hiding this comment.
These functions are so simple, I don't think they are worth the overhead. You can just call get_uid etc directly from src/shims/unix/foreign_items.rs.
| "geteuid" => { | ||
| let [] = this.check_shim_sig( | ||
| shim_sig!(extern "C" fn() -> libc::uid_t), | ||
| (link_name, abi, args), | ||
| )?; | ||
| let result = this.geteuid()?; | ||
| this.write_scalar(result, dest)?; | ||
| } |
There was a problem hiding this comment.
I am not convinced that it makes sense for Miri to support the "effective" versions of this.
| fn get_uid(&self, effective: bool) -> u32 { | ||
| let this = self.eval_context_ref(); | ||
| if !this.machine.communicate() { | ||
| return 1000; |
There was a problem hiding this comment.
You are repeating the value 1000 multiple times in this file. Please use a constant at the top of the file instead. Well, two constants: DEFAULT_UID and DEFAULT_GID.
| cfg_select! { | ||
| unix => { | ||
| // SAFETY: these functions are always safe to call. | ||
| unsafe { if effective { libc::geteuid() } else { libc::getuid() } } |
There was a problem hiding this comment.
We generally do not want shims that need to call libc directly. Quoting from our contribution guide
we will generally only add shims for functions that can be implemented in a portable way using just what is provided by the Rust standard library
The bar for shims that don't fit this pattern is extremely high. We currently have 0 of them. (The open shim uses libc constants, but only to feed into OpenOptions::custom_flags so we still don't have to call libc ourselves. This is an exceptional case because of how fundamental open is.) Miri has no interest in becoming a program that develops common abstractions over OS-specific functionality -- that's what std is for.
RalfJung
removed
the
S-waiting-on-review
rustbot
added
the
S-waiting-on-author
RalfJung
added
the
S-blocked
|
I made this a draft as it is blocked on rust-lang/libs-team#909. |
|
☔ The latest upstream changes (possibly #5431) made this pull request unmergeable. Please resolve the merge conflicts. |
Fix #5399