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.

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

Unix library: better API for "close-on-exec" over file descriptors #650

Merged
merged 16 commits into from Feb 13, 2017

Conversation

Projects
None yet
8 participants
@xavierleroy
Contributor

xavierleroy commented Jun 30, 2016

This is one possible answer to the long-standing problem described in MPR#5256.

Background

An historical accident of the Unix API is that, by default, all file descriptors remain open when a program is started via exec: the new program inherits those file descriptors. This is often a security issue, making it possible for the new program to access files and sockets it could not have otherwise, just because the original program forgot to close the corresponding file descriptors before exec.

To mitigate this problem, a file descriptor can be marked "close on exec" (e.g. with Unix.set_close_on_exec). Then, the kernel will automatically close it during the exec call, and it will not leak to the new program.

However, in multithreaded programs, a race condition exists: if a concurrently-executing thread does fork and exec after the file descriptor is created but before set_close_on_exec is applied to it, the descriptor still leaks. To address this issue, the open system call has a O_CLOEXEC flag that atomically sets "close on exec" on the file descriptor as it is created. Linux adds variants of other system calls (pipe2, dup3, accept4) that do the same for other cases of file descriptor creation. So far these syscalls are Linux-specific, but there is hope that similar functionality will be standardized one day (see http://www.austingroupbugs.net/view.php?id=411).

Proposed solution

Several approaches to the problem above were discussed in MPR#5256. This pull request implements a solution that still relies on programmer's discipline but should not introduce major incompatibilities:

  • All Unix functions that create file descriptors now have a way to specify if the descriptor is to be close-on-exec or keep-on-exec:
    • either via an optional argument ~cloexec:bool
    • or, in the case of Unix.openfile, via flags O_CLOEXEC and O_KEEPEXEC.
  • For backward compatibility, keep-on-exec is the default, but the implementation makes it trivially easy to switch to close-on-exec by default in the future.
  • The implementation of the functions honors the close-on-exec specification, using the system calls that set it atomically as the file descriptor is created, if available, and falling back on a non-atomic implementation otherwise.

Plans for the future

At some future point, it would make a lot of sense to switch the default behavior to "close-on-exec" instead of "keep-on-exec", as it is more secure. Other languages have already made that switch, e.g. Python and Ruby. If we do this now in OCaml it would break too much code. However, with the new API proposed in this pull request, programmers can start religiously annotating their Unix system calls with the appropriate ~cloexec argument. If they do this consistently on all the code that relies on file descriptor inheritance across exec, we will then be able to switch the default to "close-on-exec" and their code will keep working.

Current status

The implementation is at the prototype stage and needs more testing. (The Windows implementation wasn't even compiled, let alone tested.) I'm putting it as a pull request early so as to have a discussion on the API and on whether it's a good solution to the problem.

@toots

This comment has been minimized.

Show comment
Hide comment
@toots

toots Jun 30, 2016

Very happy to see this being worked on again. The proposed solution and PR look fine to me! I was wondering: would it be interesting to add a global switch or maybe a module functor that would allow the programmer to have a Unix module where all calls default to cloexec:true?

toots commented Jun 30, 2016

Very happy to see this being worked on again. The proposed solution and PR look fine to me! I was wondering: would it be interesting to add a global switch or maybe a module functor that would allow the programmer to have a Unix module where all calls default to cloexec:true?

@xavierleroy

This comment has been minimized.

Show comment
Hide comment
@xavierleroy

xavierleroy Jun 30, 2016

Contributor

@toots: yes, it makes a lot of sense to have programmer's control on the default value. (For example, the SecureOCaml people should probably have "close on exec" as the default.) We did something like that for hashtable randomization in OCaml 4.00. Maybe a similar API could be used here.

Contributor

xavierleroy commented Jun 30, 2016

@toots: yes, it makes a lot of sense to have programmer's control on the default value. (For example, the SecureOCaml people should probably have "close on exec" as the default.) We did something like that for hashtable randomization in OCaml 4.00. Maybe a similar API could be used here.

Show outdated Hide outdated otherlibs/unix/dup.c
{
int ret;
#ifdef F_DUPFD_CLOEXEC
if (unix_cloexec_p(cloexec))
ret = fcntl(F_DUPFD_CLOEXEC, Int_val(fd));

This comment has been minimized.

@mshinwell

mshinwell Jul 1, 2016

Contributor

I think the arguments to this fcntl call are the wrong way round.

@mshinwell

mshinwell Jul 1, 2016

Contributor

I think the arguments to this fcntl call are the wrong way round.

This comment has been minimized.

@xavierleroy

xavierleroy Jul 1, 2016

Contributor

Well spotted, thanks! Pushing a fix...

@xavierleroy

xavierleroy Jul 1, 2016

Contributor

Well spotted, thanks! Pushing a fix...

@mshinwell

This comment has been minimized.

Show comment
Hide comment
@mshinwell

mshinwell Jul 1, 2016

Contributor

I am in favour. I spotted one bug, but can have a closer look once it's been tested.

Contributor

mshinwell commented Jul 1, 2016

I am in favour. I spotted one bug, but can have a closer look once it's been tested.

@mrvn

This comment has been minimized.

Show comment
Hide comment
@mrvn

mrvn Jul 4, 2016

Contributor

I saw a recent proposal to reorganize the stdlib into a new "namespace" Std, including putting the channel functions into Std.IO. Wouldn't that also be a good way to make close_on_exec the default? As in all the functions in Std.IO will default to close_on_exec while the old functions use keep_on_exec.

Contributor

mrvn commented Jul 4, 2016

I saw a recent proposal to reorganize the stdlib into a new "namespace" Std, including putting the channel functions into Std.IO. Wouldn't that also be a good way to make close_on_exec the default? As in all the functions in Std.IO will default to close_on_exec while the old functions use keep_on_exec.

@xavierleroy

This comment has been minimized.

Show comment
Hide comment
@xavierleroy

xavierleroy Jul 4, 2016

Contributor

I fixed the Win32 implementation and added some tests. This PR is ready for review.

Concerning the API, the implementation now makes it trivial to change the default for close-on-exec. I haven't added an API function to do this yet.

Contributor

xavierleroy commented Jul 4, 2016

I fixed the Win32 implementation and added some tests. This PR is ready for review.

Concerning the API, the implementation now makes it trivial to change the default for close-on-exec. I haven't added an API function to do this yet.

Show outdated Hide outdated otherlibs/win32unix/unix.ml
let fd3 = dup ~cloexec:false fd3 in
let res = win_create_process prog args optenv fd1 fd2 fd3 in
close fd1; close fd2; close fd3;
res

This comment has been minimized.

@bobot

bobot Jul 5, 2016

Contributor

Doesn't a race condition can make the new fd1,fd2,fd3 go into another sub-process, since contrary to the unix version we are not inside a fork?

@bobot

bobot Jul 5, 2016

Contributor

Doesn't a race condition can make the new fd1,fd2,fd3 go into another sub-process, since contrary to the unix version we are not inside a fork?

This comment has been minimized.

@mrvn

mrvn Jul 5, 2016

Contributor

If dup, win_create_process or close releases the runtime lock or invokes the GC then another thread could run before the final close. If that thread then also calls create_process_aux the duplicated FDs will leak.

I'm assuming dup and close will be save here but win_create_process probably allocates something. So I agree with @bobot about the race condition. I think the dup should be moved into the win_create_process C stub and run with the runtime lock held. This would still race with non-ocaml threads forking at the same time but it's the best you can do.

@mrvn

mrvn Jul 5, 2016

Contributor

If dup, win_create_process or close releases the runtime lock or invokes the GC then another thread could run before the final close. If that thread then also calls create_process_aux the duplicated FDs will leak.

I'm assuming dup and close will be save here but win_create_process probably allocates something. So I agree with @bobot about the race condition. I think the dup should be moved into the win_create_process C stub and run with the runtime lock held. This would still race with non-ocaml threads forking at the same time but it's the best you can do.

This comment has been minimized.

@alainfrisch

alainfrisch Jul 5, 2016

Contributor

I'm assuming dup

I don't think so. Under Windows, handles are wrapped into head-allocated custom blocks, so dup can easily trigger the GC.

@alainfrisch

alainfrisch Jul 5, 2016

Contributor

I'm assuming dup

I don't think so. Under Windows, handles are wrapped into head-allocated custom blocks, so dup can easily trigger the GC.

This comment has been minimized.

@alainfrisch

alainfrisch Jul 5, 2016

Contributor

@xavierleroy What about fd leaks when win_create_process (or dup) raises an exception?

@alainfrisch

alainfrisch Jul 5, 2016

Contributor

@xavierleroy What about fd leaks when win_create_process (or dup) raises an exception?

@xavierleroy

This comment has been minimized.

Show comment
Hide comment
@xavierleroy

xavierleroy Jul 5, 2016

Contributor

@alainfrisch : Here is an explanation of the change in create_process for Win32. I think it makes it work in cases where the previous implementation would misbehave, and I don't think it can break. Assume you call Unix.create_process with redirections to three new file descriptors fd1, fd2, fd3. Further assume that fd1, fd2, fd3 are close-on-exec, like they should be so that they won't be inherited by the new process.

  • With the Unix implementation, fd1, fd2, fd3 are dup2-ed to the standard descriptors 0, 1, 2, the latter being (re-)set to keep-on-exec, so everything is fine.
  • With the old Win32 implementation, the handles fd1, fd2, fd3 are passed "as is" to the new process, which uses them as stdin, stdout and stderr. Since the handles are close-on-exec, they are actually closed (invalid) in the new process! So the new process misbehaves.
  • With the new Win32 implementation, fd1, fd2, fd3 are duplicated to inheritable handles (keep-on-exec), and the new process works as expected.

The reason why I don't think this change can break code that works today is that, in order for that code to work in Win32, the fds passed to create_process must be keep-on-exec. In this case, the new implementation behaves pretty much like the old one.

Contributor

xavierleroy commented Jul 5, 2016

@alainfrisch : Here is an explanation of the change in create_process for Win32. I think it makes it work in cases where the previous implementation would misbehave, and I don't think it can break. Assume you call Unix.create_process with redirections to three new file descriptors fd1, fd2, fd3. Further assume that fd1, fd2, fd3 are close-on-exec, like they should be so that they won't be inherited by the new process.

  • With the Unix implementation, fd1, fd2, fd3 are dup2-ed to the standard descriptors 0, 1, 2, the latter being (re-)set to keep-on-exec, so everything is fine.
  • With the old Win32 implementation, the handles fd1, fd2, fd3 are passed "as is" to the new process, which uses them as stdin, stdout and stderr. Since the handles are close-on-exec, they are actually closed (invalid) in the new process! So the new process misbehaves.
  • With the new Win32 implementation, fd1, fd2, fd3 are duplicated to inheritable handles (keep-on-exec), and the new process works as expected.

The reason why I don't think this change can break code that works today is that, in order for that code to work in Win32, the fds passed to create_process must be keep-on-exec. In this case, the new implementation behaves pretty much like the old one.

@bobot

This comment has been minimized.

Show comment
Hide comment
@bobot

bobot Jul 5, 2016

Contributor

I propose to add to the creating process functions a way to specify which fd must be passed to the created process. Perhaps with the destination file descriptor, in the same way that perform_redirections does for standard file-descriptors : (file_descr * int) list.

However it seems that there is never enough provided after fork actions (resource limitation, user change, namespace) so perhaps it should go in external library (even Core/Async_extended create_process functions were not enough in my case)

Contributor

bobot commented Jul 5, 2016

I propose to add to the creating process functions a way to specify which fd must be passed to the created process. Perhaps with the destination file descriptor, in the same way that perform_redirections does for standard file-descriptors : (file_descr * int) list.

However it seems that there is never enough provided after fork actions (resource limitation, user change, namespace) so perhaps it should go in external library (even Core/Async_extended create_process functions were not enough in my case)

@xavierleroy

This comment has been minimized.

Show comment
Hide comment
@xavierleroy

xavierleroy Jul 5, 2016

Contributor

@bobot : yes, we (still) have a race condition. At this point I don't see how to avoid it based on the CreateProcess Win32 API, but I'll keep searching on StackOverflow...

Contributor

xavierleroy commented Jul 5, 2016

@bobot : yes, we (still) have a race condition. At this point I don't see how to avoid it based on the CreateProcess Win32 API, but I'll keep searching on StackOverflow...

@xavierleroy

This comment has been minimized.

Show comment
Hide comment
@xavierleroy

xavierleroy Jul 5, 2016

Contributor

More info on CreateProcess, redirections and handle inheritance: https://blogs.msdn.microsoft.com/oldnewthing/20111216-00/?p=8873. It looks like there is a drastic solution where no handle is ever inherited, except those that become the new standard handles of the created process. This is very secure indeed but would cause problems with hypothetical applications that pass extra inherited handles through side channels (handle number as command-line parameter, for example).

Contributor

xavierleroy commented Jul 5, 2016

More info on CreateProcess, redirections and handle inheritance: https://blogs.msdn.microsoft.com/oldnewthing/20111216-00/?p=8873. It looks like there is a drastic solution where no handle is ever inherited, except those that become the new standard handles of the created process. This is very secure indeed but would cause problems with hypothetical applications that pass extra inherited handles through side channels (handle number as command-line parameter, for example).

@xavierleroy

This comment has been minimized.

Show comment
Hide comment
@xavierleroy

xavierleroy Jul 5, 2016

Contributor

@mrvn @alainfrisch : yes, the current Win32 implementation is a proof-of-concept and I'll improve it along the lines you suggest. But a race condition will remain with concurrent CreateProcess calls made directly from C. Is this good enough or should we look into the more drastic solution mentioned in my previous comment?

Contributor

xavierleroy commented Jul 5, 2016

@mrvn @alainfrisch : yes, the current Win32 implementation is a proof-of-concept and I'll improve it along the lines you suggest. But a race condition will remain with concurrent CreateProcess calls made directly from C. Is this good enough or should we look into the more drastic solution mentioned in my previous comment?

@alainfrisch

This comment has been minimized.

Show comment
Hide comment
@alainfrisch

alainfrisch Jul 5, 2016

Contributor

I'm not sure that the risk of breaking existing code with the more drastic solution is so high, esp. since our Unix module does not expose handle directly. To be on the safe side, this could always be controlled by a global switch, although this becomes more and more ugly. Ideally, there would be a way to emulate the strict mode on Unix and add an optional parameter to process-creation function to control it. But my understanding is that this is precisely what is difficult to achieve on Unix, right?

Contributor

alainfrisch commented Jul 5, 2016

I'm not sure that the risk of breaking existing code with the more drastic solution is so high, esp. since our Unix module does not expose handle directly. To be on the safe side, this could always be controlled by a global switch, although this becomes more and more ugly. Ideally, there would be a way to emulate the strict mode on Unix and add an optional parameter to process-creation function to control it. But my understanding is that this is precisely what is difficult to achieve on Unix, right?

@mrvn

This comment has been minimized.

Show comment
Hide comment
@mrvn

mrvn Jul 6, 2016

Contributor

I don't think this can ever be fully race free unless there is kernel support for actually atomically passing a specific set of FDs to the new process. Oh wait, there is since Vista.

@xavierleroy I would totaly go with the solution from the page you mentioned with an insecure fallback if the syscall isn't present. It was made for exactly this case after all.

Contributor

mrvn commented Jul 6, 2016

I don't think this can ever be fully race free unless there is kernel support for actually atomically passing a specific set of FDs to the new process. Oh wait, there is since Vista.

@xavierleroy I would totaly go with the solution from the page you mentioned with an insecure fallback if the syscall isn't present. It was made for exactly this case after all.

@xavierleroy

This comment has been minimized.

Show comment
Hide comment
@xavierleroy

xavierleroy Dec 4, 2016

Contributor

I know it's my time to move. Will try to improve createprocess, at least by moving the dup dance into the C code, so that it is more atomic and errors are handled better, and will move that we merge then.

Contributor

xavierleroy commented Dec 4, 2016

I know it's my time to move. Will try to improve createprocess, at least by moving the dup dance into the C code, so that it is more atomic and errors are handled better, and will move that we merge then.

@mshinwell

This comment has been minimized.

Show comment
Hide comment
@mshinwell

mshinwell Dec 7, 2016

Contributor

The new Jane Street "shexp" package has a carefully-written implementation for spawning Unix processes that may (or may not) be of use for reference too. I think there may be a newer version available than the current version on Jane Street's github (please ask @diml if this is of interest).

Contributor

mshinwell commented Dec 7, 2016

The new Jane Street "shexp" package has a carefully-written implementation for spawning Unix processes that may (or may not) be of use for reference too. I think there may be a newer version available than the current version on Jane Street's github (please ask @diml if this is of interest).

xavierleroy added some commits Jun 30, 2016

Experiment: better control of file descriptor inheritance across exec()
This is one possible answer to the long-standing problem described in MPR#5256:
http://caml.inria.fr/mantis/view.php?id=5256

All Unix functions that create file descriptors now have a way to specify if the descriptor is to be close-on-exec or keep-on-exec:
- either via an optional argument ~cloexec:bool
- or, in the case of Unix.openfile, via flags O_CLOEXEC and O_KEEPEXEC.

For backward compatibility, keep-on-exec is the default, but the implementation makes it trivially easy to switch to close-on-exec by default in the future.

To avoid race conditions (that can occur when a descriptor is created in keep-on-exec mode then switched to close-on-exec), the implementation uses the new system calls pipe2(), dup3() and accept4() when they are available.  So far these syscalls are Linux-specific, but there is hope that similar functionality will be standardized one day (see http://www.austingroupbugs.net/view.php?id=411).

The win32unix implementation hasn't been updated yet.
win32unix implementation of the new close-on-exec API
Tentative implementation: not tested, not even compiled.
Unix.dup implementation: missing 3rd argument to fcntl
Also: refactor the code between the cloexec/keepexec cases.
Default value of 'close on exec' is now a C variable instead of a macro
This is a first step towards making the default controllable by the user.

Also: in Unix.openfile, make sure that if O_CLOEXEC and O_KEEPEXEC are given, O_CLOEXEC wins.
Bug fixing in the Unix library for Windows
win32unix/open.c: wrong interpretation of default close-on-exec
win32unix/unix.ml: duplicate handles in create_process, for consistency with the unix implementation
tests/lib-unix: problems with deleting a file while it is open
Better implementation of Unix.createprocess for Win32
The "dup" operations are done in C for better atomicity.
FDs are properly closed in case of error.
Update the Win32 implementation of unix.ml
Use ~cloexec:true where appropriate.
Cherry-pick bb96c0b from unix/unix.ml (Fix #5421: do not leak fds in various open_proc* functions).
Show outdated Hide outdated otherlibs/win32unix/unix.ml
close out_read; close in_write; close err_write;
begin
try
open_proc cmd (Some env) (Process_full(inchan, outchan, errchan))

This comment has been minimized.

@dra27

dra27 Jan 4, 2017

Contributor

I think this should be (Some(make_process_env env))

@dra27

dra27 Jan 4, 2017

Contributor

I think this should be (Some(make_process_env env))

xavierleroy added some commits Jan 4, 2017

Fix type error in open_process_full
Note: the test lib-unix/redirections.ml sometimes fails (mingw32 port).  To be investigated.
Test redirections.ml incorrect under Win32
Closing the standard input causes errors in open_process_out
because the Win32 CreateProcess syscall demands that all standard handles
are valid.

Strangely, the error is semi-random and occurs about once every 3 runs.
@xavierleroy

This comment has been minimized.

Show comment
Hide comment
@xavierleroy

xavierleroy Jan 23, 2017

Contributor

I am offering again this pull request for review and discussion of integration.

Since the previous discussion in July, I improved the Win32 implementation of create_process (fixing FD leaks on errors and making it less sensitive to race conditions), added some tests and some documentation.

As to why this PR should be merged, please refer to the explanations at the beginning of this PR. In a nutshell, it is a conservative extension of the Unix API that remains backward-compatible but makes it easy to create file descriptors in close-on-exec mode (like they should be), and rewards the user of the new ?cloexec:bool optional arguments with an implementation that is as atomic as the underlying OS supports.

Contributor

xavierleroy commented Jan 23, 2017

I am offering again this pull request for review and discussion of integration.

Since the previous discussion in July, I improved the Win32 implementation of create_process (fixing FD leaks on errors and making it less sensitive to race conditions), added some tests and some documentation.

As to why this PR should be merged, please refer to the explanations at the beginning of this PR. In a nutshell, it is a conservative extension of the Unix API that remains backward-compatible but makes it easy to create file descriptors in close-on-exec mode (like they should be), and rewards the user of the new ?cloexec:bool optional arguments with an implementation that is as atomic as the underlying OS supports.

@toots

This comment has been minimized.

Show comment
Hide comment
@toots

toots Jan 23, 2017

Would love to see this one merged. The benefit/rational is quite clear.

toots commented Jan 23, 2017

Would love to see this one merged. The benefit/rational is quite clear.

@dra27

This comment has been minimized.

Show comment
Hide comment
@dra27

dra27 Jan 24, 2017

Contributor

I have no issue with this being merged - it just always feels strange to approve your changes! 😄 @alainfrisch also happy from a Windows perspective?

Contributor

dra27 commented Jan 24, 2017

I have no issue with this being merged - it just always feels strange to approve your changes! 😄 @alainfrisch also happy from a Windows perspective?

@alainfrisch

This comment has been minimized.

Show comment
Hide comment
@alainfrisch

alainfrisch Jan 24, 2017

Contributor

also happy from a Windows perspective?

Is there anything specific to Windows (apart from a different implementation)?

The implementation of the functions honors the close-on-exec specification, using the system calls that set it atomically as the file descriptor is created, if available, and falling back on a non-atomic implementation otherwise.

I feel a bit uncomfortable with this silent fall back, considering security implications. A missing detection at configure-time (for whatever reason) could lead to shipping a more vulnerable executable. Perhaps a "runtime warning", controlled by the same flag as the one for non-closed channels (now disabled by default), might help, but it would also probably go unnoticed. What about a strict mode (that can be enabled by the application) which force functions to fail if their atomic cloexec version is not available when the user ask for it? Or exposing flags to allow the programmer checking if the atomic functions are available?

Contributor

alainfrisch commented Jan 24, 2017

also happy from a Windows perspective?

Is there anything specific to Windows (apart from a different implementation)?

The implementation of the functions honors the close-on-exec specification, using the system calls that set it atomically as the file descriptor is created, if available, and falling back on a non-atomic implementation otherwise.

I feel a bit uncomfortable with this silent fall back, considering security implications. A missing detection at configure-time (for whatever reason) could lead to shipping a more vulnerable executable. Perhaps a "runtime warning", controlled by the same flag as the one for non-closed channels (now disabled by default), might help, but it would also probably go unnoticed. What about a strict mode (that can be enabled by the application) which force functions to fail if their atomic cloexec version is not available when the user ask for it? Or exposing flags to allow the programmer checking if the atomic functions are available?

@xavierleroy

This comment has been minimized.

Show comment
Hide comment
@xavierleroy

xavierleroy Jan 31, 2017

Contributor

@alainfrisch : currently only Linux provides the full set of system calls needed to get atomicity in all cases. With your proposal you'd get run-time warnings or even errors on any other platform. I don't think this is a good deal.

At any rate: my proposal in this PR can be improved in several directions, many of them listed in the discussion. I still think it is a solid first step that improves the current situation already. Can we PLEASE agree to merge it NOW and AS IS ?

Contributor

xavierleroy commented Jan 31, 2017

@alainfrisch : currently only Linux provides the full set of system calls needed to get atomicity in all cases. With your proposal you'd get run-time warnings or even errors on any other platform. I don't think this is a good deal.

At any rate: my proposal in this PR can be improved in several directions, many of them listed in the discussion. I still think it is a solid first step that improves the current situation already. Can we PLEASE agree to merge it NOW and AS IS ?

@alainfrisch

This comment has been minimized.

Show comment
Hide comment
@alainfrisch

alainfrisch Jan 31, 2017

Contributor

May I still suggest to amend to documentation of Unix.set_close_on_exec to be more precise about the (potential lack of) atomicity guarantees, even when the new optional argument is used?

Contributor

alainfrisch commented Jan 31, 2017

May I still suggest to amend to documentation of Unix.set_close_on_exec to be more precise about the (potential lack of) atomicity guarantees, even when the new optional argument is used?

@xavierleroy

This comment has been minimized.

Show comment
Hide comment
@xavierleroy

xavierleroy Feb 3, 2017

Contributor

@alainfrisch your wish for documentation is my command. Are we good to go now?

Contributor

xavierleroy commented Feb 3, 2017

@alainfrisch your wish for documentation is my command. Are we good to go now?

@alainfrisch

This comment has been minimized.

Show comment
Hide comment
@alainfrisch

alainfrisch Feb 3, 2017

Contributor

Ok for me. I'm glad I asked, because I thought, after your previous note, that only Linux provided the atomicity guarantee (but Windows does as well). I also missed the fact that only non-OCaml threads were affected by the lact of atomicity.

Contributor

alainfrisch commented Feb 3, 2017

Ok for me. I'm glad I asked, because I thought, after your previous note, that only Linux provided the atomicity guarantee (but Windows does as well). I also missed the fact that only non-OCaml threads were affected by the lact of atomicity.

Update the documentation about the ~cloexec flag
Unlike claimed earlier, Windows has one nonatomic behavior in Unix.createprocess that enables a C thread to leak a close-on-exec descriptor.
@xavierleroy

This comment has been minimized.

Show comment
Hide comment
@xavierleroy

xavierleroy Feb 6, 2017

Contributor

On second thoughts, the Windows implementation of Unix.createprocess still has a race condition with other C threads that can lead to a close-on-exec leak. Updated the documentation again.

Are we good to go now?

Contributor

xavierleroy commented Feb 6, 2017

On second thoughts, the Windows implementation of Unix.createprocess still has a race condition with other C threads that can lead to a close-on-exec leak. Updated the documentation again.

Are we good to go now?

@damiendoligez damiendoligez merged commit ab4e3be into trunk Feb 13, 2017

3 checks passed

continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
continuous-integration/travis-ci/push The Travis CI build passed
Details
@toots

This comment has been minimized.

Show comment
Hide comment
@toots

toots commented Feb 13, 2017

yey!

@xavierleroy

This comment has been minimized.

Show comment
Hide comment
@xavierleroy

xavierleroy Feb 13, 2017

Contributor

I'm relieved to be able to mark MPR#5256 as "resolved" -- it's been bothering me for almost 6 years...

Contributor

xavierleroy commented Feb 13, 2017

I'm relieved to be able to mark MPR#5256 as "resolved" -- it's been bothering me for almost 6 years...

@xavierleroy xavierleroy deleted the cloexec branch Feb 13, 2017

@damiendoligez

This comment has been minimized.

Show comment
Hide comment
@damiendoligez

damiendoligez Feb 14, 2017

Member

The test breaks on cygwin32 on our CI machine for some very weird reason (cygwin fails to fork because of some Windows permission problem).

I tried updating Windows and Cygwin, rebooting to get rid of old processes, and rebasing. I don't know what to do now. @alainfrisch @dra27 any idea?

Member

damiendoligez commented Feb 14, 2017

The test breaks on cygwin32 on our CI machine for some very weird reason (cygwin fails to fork because of some Windows permission problem).

I tried updating Windows and Cygwin, rebooting to get rid of old processes, and rebasing. I don't know what to do now. @alainfrisch @dra27 any idea?

@xavierleroy

This comment has been minimized.

Show comment
Hide comment
@xavierleroy

xavierleroy Feb 14, 2017

Contributor

See discussion on caml-devel and MPR#7484. It's an orthogonal problem that is exercised by the new tests included in the present PR.

Contributor

xavierleroy commented Feb 14, 2017

See discussion on caml-devel and MPR#7484. It's an orthogonal problem that is exercised by the new tests included in the present PR.

mfp added a commit to ocsigen/lwt that referenced this pull request Mar 26, 2017

Make Lwt_unix.openfile's O_CLOEXEC/O_KEEPEXEC more future-proof.
The default close-on-exec behavior for Unix.openfile might change in future
OCaml releases, refer to ocaml/ocaml#650

This patch ensures Lwt_unix.openfile's default behavior will match
Unix.openfile (and also keep consistency w.r.t. it between Win32 and other
platforms).

camlspotter pushed a commit to camlspotter/ocaml that referenced this pull request Oct 17, 2017

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment