Remove direct usage of fork(2) in most tests, use trampolined execution instead #113
Conversation
+ Remove unnecessary usages of fork in tests.
pawelchcki
force-pushed
the
pawel/spawn_ld_preload
branch
from
88ea622
to
ec222fb
Compare
|
Thanks @paullegranddc and @r1viollet for the reviews 🙇 |
| pub fn recv_passed_fd() -> Option<OwnedFd> { | ||
| let val = env::var(ENV_PASS_FD_KEY).ok()?; | ||
| let fd: RawFd = val.parse().ok()?; | ||
|
|
||
| // check if FD number is valid | ||
| nix::fcntl::fcntl(fd, nix::fcntl::FcntlArg::F_GETFD).ok()?; | ||
|
|
||
| Some(unsafe { OwnedFd::from_raw_fd(fd) }) | ||
| } |
There was a problem hiding this comment.
This reminds me that we should probably close all non passed file descriptors, in order to not keep alive the applications files and socket open, since the sidecar could outlive the parent process
There was a problem hiding this comment.
That is the beauty of using exec! we don't have to!
Assuming we can trust most cases where people might have some sockets, and if they don't like those sockets to be shared with other processes they will do set CLOSEXEC
....
Now I realized that its the APM world we're talking about here :| - and maybe we should close them
| enum ChildStdio { | ||
| Inherit, | ||
| Owned(OwnedFd), | ||
| Ref(libc::pid_t), |
There was a problem hiding this comment.
A raw fd should be of type std::os::fd::RawFd, libc::pid_t is confusing
| if fd.as_raw_fd() >= 0 && fd.as_raw_fd() <= libc::STDERR_FILENO { | ||
| Ok(ChildStdio::Owned(fd.try_clone()?)) | ||
| } else { | ||
| Ok(ChildStdio::Ref(fd.as_raw_fd())) | ||
| } |
There was a problem hiding this comment.
I don't understand this one. Why couldn't we clone non stdin/stdout/stderr file descriptors?
There was a problem hiding this comment.
this code follows what Command::spawn is doing. Also since stdio fd's are not CLOSEXEC by default then we don't need to dupe them.
| // we're stripping the close on exec flag from the FD | ||
| // to ensure we will not modify original fd, whose expected lifetime is unknown | ||
| // we should clone the FD that needs passing to the subprocess, keeping its lifetime | ||
| // as short as possible |
There was a problem hiding this comment.
I disagree with this, on fork all of the processes fd gets cloned, so I am not sure what cloning an fd to a "smaller" lifetime improves..
On the CLOEXEC argument I am split.
- Passing a fd with the flag set should be an error since the creator of the fd specified that they don't want them to get passed to forks
- On the other hand, by default
try_cloneon io-lifetimes fds adds this flag https://doc.rust-lang.org/src/std/os/fd/owned.rs.html#94 . But IMO the fix should be either stopping to clone OwnedFds all over the place and use Borrowed ones instead, or use our own clone function that skips this flag.
What does this PR do?
This addresses long standing issue with fork being inherently unsafe and unreliable way to run tests - and sidecar itself on production environments.
To enable running tests directly on linux - a new LD_PRELOAD based trampoline was added. It will automatically kick off when the spawn_worker crate is used from a linked executable.
However 1 caveat exists that enables this. The tests need to be compiled with "-rdynamic" flag which will export symbols so that they are available for LD_PRELOAD to look up and execute
Motivation
Evils of fork
Additional Notes
Fork is evil
How to test the change?