search

baudehlo / node-fs-ext

Extras missing from node's fs module
MIT License
101 stars 45 forks source link

Add `fcntl` bindings for FD_CLOEXEC #32

Closed timbertson closed 9 years ago

timbertson commented 10 years ago

There's a lot of possible functionality in fcntl, but the big one missing from nodejs is controlling the CLOEXEC flag (close-on-exec). Without this, it's basically impossible to prevent (potentially security-sensitive) open file descriptors leaking into any child process.

This PR adds support for fcntl, but only as much as is required to control the FD_CLOEXEC flag.

I have not tried building or testing it on Windows.

baudehlo commented 10 years ago

Without this, it's basically impossible to prevent (potentially security-sensitive) open file descriptors leaking into any child process

For what it's worth, this has been fixed since Node v0.10. See the docs for child_process.spawn. By default it only sends stdin/out/err to the child process.

Haven't had chance to read the commit yet, but thought you should know that.

timbertson commented 10 years ago

@baudehlo are you sure? I'm using 0.10.26, and definitely see this issue still with stdio = [0,1,2]. I even tried setting a stdio argument of [0,1,2, <50x'ignore'>], and my FD still leaks (even when it's numbered 10).

Looks like it'll be fixed in node 0.12 (https://github.com/joyent/libuv/commit/6abe1e4), at least for results of fs.open() (but not necessarily for other FDs).

timbertson commented 10 years ago

Here's some sample code showing the leakage, in case you're curious: https://gist.github.com/gfxmonk/848fcd704f6966ddb43f

baudehlo commented 9 years ago

Sorry about the LONG delay on this - if you still want this merged, it needs to be updated to the nan API. See https://github.com/rvagg/nan for documentation. It should be a fairly minor change.

timbertson commented 9 years ago

Cool, I'll check out nan and update this PR when I've got a chance.

timbertson commented 9 years ago

OK, updated. I mostly just followed what other similar functions are already doing when it comes to nan, so let me know if I've missed something. The tests still pass, which is a good sign :)

baudehlo commented 9 years ago

I don't know nan at all (yet), so I'm just going to merge this, and probably push a new npm release too.