joyent / libuv

Go to
https://github.com/libuv/libuv
3.27k stars 654 forks source link

darwin: spawn new process with POSIX_SPAWN_CLOEXEC_DEFAULT #1483

Open bnoordhuis opened 10 years ago

bnoordhuis commented 10 years ago

On Linux (and FreeBSD too now, I think), libuv opens file descriptors with O_CLOEXEC. On other platforms however, there is a race window between the creation of the file descriptor and setting its FD_CLOEXEC flag. As a result, uv_spawn() may leak file descriptors into the new process and that's a potential security issue.

OS X has a POSIX_SPAWN_CLOEXEC_DEFAULT posix_spawnattr_t flag that could mitigate that. There is a gotcha however: you use posix_spawn_file_actions_addinherit_np() for file descriptors that the new process should inherit but that function only accepts file descriptors < OPEN_MAX (10,240.)

Note that this could perhaps be solved generically with a pthread_atfork() handler but I think that would require libuv to store file descriptors in a global structure and exclusively use atomic operations to update that structure. A secondary issue with that approach is that it gives odd results when RLIMIT_NOFILE has been lowered. You won't be able to close file descriptors above the limit.

See also this comment.

txdv commented 9 years ago

How do you come up with these?

bnoordhuis commented 9 years ago

The angels talk to me.

(Actually, the thought popped in my head while going over open rust issues.)

vtjnash commented 9 years ago

Is there a reason that bInheritHandles is being set on Windows during CreateProcess? I would not expect to want to do this on Windows either.

https://github.com/libuv/libuv/blob/01bbf6fb1ccba74575fa232ff69bf1a00431c37b/src/win/process.c#L1082 https://msdn.microsoft.com/en-us/library/windows/desktop/ms682425(v=vs.85).aspx

saghul commented 9 years ago

@vtjnash please open an issue over at libuv/libuv, we moved over there a while ago :-)

vtjnash commented 9 years ago

for the windows bit, i can just open a PR over there. just figured i would try to confirm first whether this was intentional.

jaykrell commented 6 years ago

Note that Windows mechanism here is flawed in one or two ways, and there is a workaround. Here are the two problems: Even if you give an explicit list of handles, they must be marked as inheritable. Other code in the process calling CreateProcess in the old way without a list, will still inherit all inheritable handles.

The workaround for explicit inheritance is to first create a dummy parent process without inheritance, duplicate handles into it, and then specify it as the parent in the real CreateProcess.