Implement unix pipes in libnative #12103
Conversation
|
The naming seems quite confusing, since "Unix pipes" normally refers to fds returned by the pipe() function, while this pull request instead implements Unix domain sockets (fds returned by socket() or socketpair() with AF_UNIX). |
|
That's why I opened #12093 about the naming problem. |
| let s: &mut libc::sockaddr_un = unsafe { cast::transmute(&mut storage) }; | ||
|
|
||
| let len = addr.len(); | ||
| if len > s.sun_path.len() - 1 { |
There was a problem hiding this comment.
This is something of an off-by-one error. On Linux, the path doesn't have to be zero-terminated if it's sizeof(s.sun_path) bytes long while on (most of?) the BSDs it's possible to use a path that's longer than that by passing sizeof(struct sockaddr_un) + extra_bytes as the addrlen argument.
(On that subject, I found a nice little local exploit when checking whether that works on XNU: https://gist.github.com/bnoordhuis/8882643)
There was a problem hiding this comment.
Another issue is that it won't allow abstract namespace sockets because those start with a zero byte. They're Linux-only however so maybe they're out of scope.
There was a problem hiding this comment.
Interesting, so in general this should probably malloc a sockaddr_un with the header + entire length of the CString? I figure for platform compatibility it could null terminate it, and then the length would be the size of the entire structure? (header + cstring length + 1 for null)
There was a problem hiding this comment.
Taking a look at the libuv implementation, it looks like the requested path is truncated to sizeof(s.sun_path) (just for comparison). Do you know if that was intentional or "just because"?
There was a problem hiding this comment.
Interesting, so in general this should probably malloc a sockaddr_un with the header + entire length of the CString
That's mostly right. There's still an upper limit that's lower than PATH_MAX, I think it's usually 256 or 1024 bytes, but you could leave it to the user to deal with that.
Taking a look at the libuv implementation, it looks like the requested path is truncated to sizeof(s.sun_path) (just for comparison). Do you know if that was intentional or "just because"?
Pretty much just because, I'm afraid. I wrote libuv's initial pipes implementation under time pressure with the idea of coming back to it once long paths started becoming an issue. But it never became an issue and there were always more critical bugs to squash so yeah...
There was a problem hiding this comment.
The sun_path element is limited to 108 or 104 characters, depending on the platform. On Linux, it seems it must be null-terminated (and the terminating character is counted in the size): http://man7.org/linux/man-pages/man7/unix.7.html
There was a problem hiding this comment.
Hm after thinking about this, I think I'm tempted to leave this as-is for the sake of simplicity for now. I'm thinking we can revisit if this comes up to be a problem in the future, but I can also open an issue on it.
|
Left some comments but LGTM in general. It leaks file descriptors across forks but that issue is not unique to this PR. Are there plans to address that? I could help out, I've dealt with that before. |
|
I am indeed very interested about leaking file descriptors. Right now I don't think that we have plans to make the runtime itself fork-safe, but I don't think that we should be leaking file descriptors if you're spawning processes (which uses |
|
That could be an interesting problem to solve and guaranteee by the compiler. Right now, we have to use flags like FD_CLOEXEC or O_CLOEXEC on Linux to prevent file descriptors from leaking, but I wonder if the concept of ownership could be implemented for file descriptors, handles, and have the compiler warn if they're leaking at fork time. |
|
I don't really understand where the leaks are coming from, so this may be irrelevant, but we do try to close all extraneous file descriptors after a fork (https://github.com/mozilla/rust/blob/master/src/libnative/io/process.rs#L462-L466) |
|
The leaks come from the fact that another thread might call fork() between the time you open an fd, and the time you set FD_CLOEXEC on it. Closing all descriptors before exec still has the problem that if the parent closes that fd, the close may gets delayed indefinitely until the child process runs that code (which may never happen if someone happens to send it a SIGSTOP at the right time), which is bad if the close has side effects (e.g. notifying the other end of a socket or pipe, or releasing a lock on a file). Anyway, for the code in this pull request and sockets in general, one can just use SOCK_CLOEXEC on platforms providing it. |
|
What @bill-myers said, there is a race with fork() calls in other threads. FreeBSD 10 and non-ancient Linux have system calls that atomically set the close-on-exec flag: open(O_CLOEXEC), connect(SOCK_CLOEXEC), accept4(), etc. On Linux, those system calls appeared around 2.6.27. That's not that long ago: RHEL 5, the de facto baseline for enterprise support, ships 2.6.18 and is supported until 2017. Having a hard dependency on a newer kernel will be problematic for many enterprise users. What libuv does is try the newer system calls first and fall back to the old broken-by-design system calls when they're not supported. Apropos walking the file descriptor table with getdtablesize(), that helps somewhat but it's easy to subvert by lowering RLIMIT_NOFILE: any open file descriptors above the new limit remain open and will leak into the new process. |
|
Hm, I'll open an issue on the leaking file descriptors for now, we currently don't open anything with |
|
#12148 - file descriptors possibly leaking |
|
ping r? |
The windows named pipes implementation will have almost nothing to do with unix pipes, so I think it's best if they live in separate files.
* Implementation of pipe_win32 filled out for libnative * Reorganize pipes to be clone-able * Fix a few file descriptor leaks on error * Factor out some common code into shared functions * Make use of the if_ok!() macro for less indentation Closes rust-lang#11201
There's a few parts to this PR * Implement unix pipes in libnative for unix platforms (thanks @Geal!) * Implement named pipes in libnative for windows (terrible, terrible code) * Remove `#[cfg(unix)]` from `mod unix` in `std::io::net`. This is a terrible name for what it is, but that's the topic of #12093. The windows implementation was significantly more complicated than I thought it would be, but it seems to be passing all the tests. now. Closes #11201
…erives, r=jonas-schievink feat: diagnose unresolved derive macros 
Do not suggest `bool::then()` and `bool::then_some` in `const` contexts Fix rust-lang#12103 changelog: [`if_then_some_else_none`]: Do not trigger in `const` contexts
There's a few parts to this PR
#[cfg(unix)]frommod unixinstd::io::net. This is a terrible name for what it is, but that's the topic of io::net::unix needs to be renamed #12093.The windows implementation was significantly more complicated than I thought it would be, but it seems to be passing all the tests. now.
Closes #11201