Skip to content
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.

Sign up for GitHub

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

Jump to bottom

pipe2 doesn't set flags atomically? #414

Closed
oconnor663 opened this issue Sep 5, 2016 · 6 comments
Closed

pipe2 doesn't set flags atomically? #414

oconnor663 opened this issue Sep 5, 2016 · 6 comments

Comments

@oconnor663
Copy link
Contributor

It looks like the pipe2 function is implemented as a call to regular pipe followed by separate calls to fcntl (for compatibility with systems that don't support pipe2?). I think that's fine for O_NONBLOCK, but not for O_CLOEXEC. If a multithreaded program forks in between the creation of a pipe and when O_CLOEXEC is set, the child will inherit pipes that it isn't supposed to.

Is it possible to call the real pipe2 libc function where it's supported, and to fall back to pipe only when necessary? I think this is what libstd does.
@posborne
Copy link
Member

posborne commented Sep 6, 2016

Is it possible to call the real pipe2 libc function where it's supported, and to fall back to pipe only when necessary? I think this is what libstd does.

I have no objection to this. Mind opening up a PR?

@oconnor663
Copy link
Contributor Author

I'd love to put it together, but I could use your advice about the right way to do it. Rust's libstd uses some kind of weak binding backed by function called dlsym. I'm don't know much about what that function's doing under the covers, but if you think that's the right way to go I'll read more about it. Are there any examples in nix already of doing this kind of capability detection?

@posborne
Copy link
Member

posborne commented Sep 6, 2016

Personally, at least for the first pass, I am OK only supporting linux 2.6.27+ in nix as long as we make a note that further work would be required to support older kernels. Alternatively, borrowing that macro/logic from libstd might also be an acceptable solution.

dlsym in this context is just seeing if the libc in used contains specific symbols at runtime.

@oconnor663
Copy link
Contributor Author

Interesting, it looks like musl already implements a similar fallback: http://git.musl-libc.org/cgit/musl/tree/src/unistd/pipe2.c

@oconnor663 oconnor663 mentioned this issue Sep 14, 2016
Merged
homu added a commit that referenced this issue Sep 17, 2016
@homu
Auto merge of #427 - oconnor663:pipe2, r=fiveop
6ea8f7f 
call pipe2 directly on Linux

A first shot at fixing #414. This approach keeps the old implementation for platforms other than `notbsd`, because `libc` only exposes `pipe2` in the `notbsd` module.

I've tested this by hand on my Linux machine in a couple ways:

- Create a toy program that opens a pipe and passes it to `cat`, which hags if `O_CLOEXEC` doesn't get set properly. Confirm that it doesn't hang, but that it does if I pass `0` as the flags to `libc::pipe2`.
- Delete the new implementation entirely, and delete the `cfg` guards on the old implementation, and confirm that above is still true.

I haven't actually tested a cross compilation build though. Is there a standard way to do that?
@fiveop
Copy link
Contributor

fiveop commented Sep 18, 2016

If I understand it, the situation cannot be improved, from where your PR got us. Can we close this now?

@oconnor663
Copy link
Contributor Author

Yep. Closing.

Does libc have any plans to support pipe2 on BSD platforms? What's the broader future of detecting this sort of thing dynamically?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
3 participants
@posborne @fiveop @oconnor663