Add the Linux prctl syscall #1550
Conversation
There was a problem hiding this comment.
The first question is: "if the prctl crate is good enough, then why does Nix need these bindings too?"
Secondly, I don't think the proposed API is very good. Those four c_ulong arguments look like they have different meanings for different PrctlOption values, and that's not very Rusty. One alternative would be to specify the option as an enum with a variable number arguments for each value. Another, as you suggested, would be to turn each prctl option into its own function. Not being familiar with prctl myself, I can't say which is best.
|
I forgot to mention the related issue (#601) in my first post. To answer your first question, I think that it would be nice to have prctl implemented in Nix. There are other crates for inotify and epoll too, but if I can get all three of them in one crate then that is my preferred option. I agree that my pull request isn't very good. But I am willing to put in the work to bring it up to Nix's standards. The weird thing about prctl is that some options can return data through the function call and other options return data to the location pointed to by one of the arguments, this behaviour makes it quite unsafe. Hence why I think that making wrapper functions is the best option. I'll work a bit on this PR and come back with the better solution. |
|
What is your opinion on the solution now? I have not implemented all the options yet as I wanted to hear your opinion first. Possibly the prctl function and PrctlOption enum do not have to be public anymore, if all possibilities are implemented as separate functions. |
|
Thank you for your feedback! I have a couple more of the options to add (and fix the tests), but I hope to get some time to get it done this week. What is your opinion on the more exotic options that are available through prctl, should I implement them as well in this PR? Like architecture specific options, that I don't have the hardware to test with (powerpc, ia, mips etc.) and options to control capabilities where there are recommended higher-level libraries like libcap. I have implemented the process-control options and left out the more exotic options for now. |
|
@asomers I can't seem to figure out what is going wrong in the builds. Do you have any idea? Could you point me in the right direction? The errors are in architectures I don't have access to, namely MIPS, MIPS64, mipsel, arm gnueabi and armv7 gnueabihf. It appears to be pointer related, as the error EFAULT from prctl indicates that it is an invalid address. I pass the pointer to prctl as a Do you know of any examples in the rest of the Nix code where a pointer is passed to and written by the syscall? |
|
uclibceabihf is failing through no fault of your own, and hopefully it will soon be fixed on the master branch. arm gnueabi and armv7 gnueabihf are tested using QEMU. QEMU doesn't implement all syscalls. If prctl is one, then you can add |
|
@asomers I have fixed the tests and completed a bunch more options. I think this PR is ready if you are satisfied with it. |
|
@asomers What do you think? Is this PR ready or is there anything I should change? |
|
I'm interested in this too, particularly setting no new privileges. |
There was a problem hiding this comment.
All of the tests seem to assume that the default settings will be present. Are you sure that will be true, or could it depend on the environment? Also, is it possible for any of them to interfere with other tests, including in other modules? It makes me nervous that they all change global settings.
I have changed the tests so that they do not assume that the defaults are set. And I've also added cleanup that resets the settings to what they were before the test.
Some of these affect the process and not only the thread, so it might be possible that these tests affect other tests. |
|
I see, you did move them into their own process. That looks good. Would you mind squashing your commits now? |
DOhlsson
force-pushed
the
prctl
branch
2 times, most recently
from
19ad914 to
60acaa1
Compare
|
@asomers I have squashed the commits. I also added a line to the changelog. |
|
@asomers is it ready to merge or is there anything else I should do? |
src/sys/prctl.rs
Outdated
| /// Set the name of the calling thread. Strings longer than 15 bytes will be truncated. | ||
| pub fn set_name(name: &str) -> Result<()> { | ||
| let name = CString::new(name.as_bytes()).unwrap(); | ||
|
|
||
| let res = unsafe { libc::prctl(libc::PR_SET_NAME, name.as_ptr(), 0, 0, 0) }; | ||
|
|
||
| Errno::result(res).map(drop) | ||
| } |
There was a problem hiding this comment.
The str input plus the automatic truncation don't seem like they go well together, as the truncation could cut off part of a character.
I wonder if it would be better to just have the input argument be &[u8]?
There was a problem hiding this comment.
@eric-seppanen Changing to &[u8] would not prevent characters to be cut off in the middle? If the user just does an as_bytes() on their string it could still truncate in the middle of a character.
There was a problem hiding this comment.
That's true. My concern was that &str implies that unicode will be handled appropriately, which isn't a promise this function can make.
Maybe &OsStr would be the right choice? It implies that there's a string, but clearly expresses to the caller that the handling of the bytes is os-specific.
There was a problem hiding this comment.
I don't think there is any convenient way to convey that the input string could be truncated. In the end I settled on CStr because it hints at the underlying C code and because it's the type of string least likely to be UTF-8.
src/sys/prctl.rs
Outdated
| /// Return the name of the calling thread | ||
| pub fn get_name() -> Result<String> { | ||
| // Size of buffer determined by linux/sched.h TASK_COMM_LEN | ||
| let buf = [0u8; 16]; | ||
|
|
||
| let res = unsafe { libc::prctl(libc::PR_GET_NAME, &buf, 0, 0, 0) }; | ||
|
|
||
| let len = buf.iter().position(|&c| c == 0).unwrap_or(buf.len()); | ||
| let cstr = CStr::from_bytes_with_nul(&buf[..=len]).map_err(|_| Errno::EINVAL)?; | ||
| let name = cstr.to_str().map_err(|_| Errno::EINVAL)?; | ||
|
|
||
| Errno::result(res).map(|_| name.to_owned()) | ||
| } |
There was a problem hiding this comment.
If set_name were to switch to &[u8] then perhaps this should change the return value from String to Box<[u8]>?
Then the caller could decide how to handle non-UTF-8 data.
There was a problem hiding this comment.
On second thought, Vec<u8> might be better than Box<[u8]>, to make it easier for the caller to do String::from_utf8(name) afterward.
There was a problem hiding this comment.
Same here as on the previous comment. I settled for CString because then the user will be forced to handle the utf8 conversion themselves.
DOhlsson
force-pushed
the
prctl
branch
2 times, most recently
from
03b5a03 to
d15b2ab
Compare
|
@asomers I have rebased this branch onto latest commit from master. Can this PR be merged as is, or would you like me to change anything else? |
| let len = buf.iter().position(|&c| c == 0).unwrap_or(buf.len()); | ||
| let name = CStr::from_bytes_with_nul(&buf[..=len]).map_err(|_| Errno::EINVAL)?; |
There was a problem hiding this comment.
Ideally you could do this instead. But that would require raising the MSRV to 1.69.0, so probably we shouldn't do it.
| let len = buf.iter().position(|&c| c == 0).unwrap_or(buf.len()); | |
| let name = CStr::from_bytes_with_nul(&buf[..=len]).map_err(|_| Errno::EINVAL)?; | |
| let name = CStr::from_bytes_until_nul(&buf[..]); |
I have added support for the prctl syscall.
However, I am not sure that this is the right way to do it. Another way would be to wrap each possible flag to prctl as it's own function, that way it would be more similar to how the prctl crate does it. Wrapping the different options would be safer, since some options to prctl return data through pointers.
What do you think?