-
-
Notifications
You must be signed in to change notification settings - Fork 9
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
Soundness issue with std::env::remove_var #9
Comments
Thanks for bringing this to my attention. I don't really see a way to fix this. Pretty much, only options are:
I don't want to drop this functionality because it's important if you're going to spawn other processes. And in practice, it's not going to be such a big deal, since readiness notification is something that most programs only do on start-up. So I'll probably update the docs in the next version, and begrudgingly mark the APIs as unsafe in 0.5. |
Yes, this is hard to fix correctly. I think even having env manipulation only during startup can be a potential problem. In programs that use tokio threads can be spawned very early (e.g. for I/O, setting up the sockets or other file I/O). Just out of interest? Thanks for your help and for your incredibly quick response. |
Yeah, I don't think this is necessarily going to be an issue with
Environment variables normally get inherited by child processes, so a program will want to prevent any children from talking to systemd on its behalf. That's not just about readiness notifications, but also things like the file descriptor store. But in practice I think it's mostly to prevent a child process from believing it was started by systemd, and messing things up without actually trying to be malicious. |
Ah, thanks for explaining this. Yes, this makes sense. |
I think the opposite will usually be the case, especially with code that uses I suggest adding a function to Thoughts? |
My understanding is that multiple threads can be running at tokio main start, but they wouldn't do anything unless main schedules another async task. But couldn't we check somehow if the current process has multiple threads? |
How about an API like this, where SdNotify::new().notify(&[]);
let sd = unsafe { SdNotify::new_with_unset_env() };
sd.notify(&[]); CC @cbranch since you're probably using this crate. |
This does not fix the actual issue but alerts users to this unsoundness issue. Also changes the examples so that the parameter isn't set to `true` mindlessly. CC lnicola#9
They need to be `unsafe` because `std::env::remove_var` is unsafe. Fixes lnicola#9.
This does not fix the actual issue but alerts users to this unsoundness issue. Also changes the examples so that the parameter isn't set to `true` mindlessly. CC lnicola#9
They need to be `unsafe` because `std::env::remove_var` is unsafe. Fixes lnicola#9.
As described in the documentation,
std::env::remove_var
will become unsafe in a future version of Rust.Even today the use of
std::env::remove_var
is unsound in multithreaded programs and can cause UB. See the issue for more information. I think this is a problem forsd-notify
, because threads can be accessing the environment, whilesd-notify
callsremove_var
.Are there any plans how to handle this in
sd-notify
, yet?Thanks for your opinions :)
The text was updated successfully, but these errors were encountered: