janet-lang / janet

A dynamic language and bytecode vm
https://janet-lang.org
MIT License
3.45k stars 223 forks source link

Avoid prematurely closing file descriptors when redirecting IO #1201

Closed pyrmont closed 1 year ago

pyrmont commented 1 year ago

Janet raises an error if os/execute is called with the same stream being used to redirect both STDOUT and STDERR:

(def dn1 (os/open "/dev/null" :w))
(def p1 (os/execute ["ls"] :px {:out dn1})) # will not raise an error

(def dn2 (os/open "/dev/null" :w))
(def p2 (os/execute ["ls"] :px {:out dn2 :err dn2})) # will raise an error

This problem currently affects JPM when installing with --silent (e.g. jpm --silent install judge). This PR prevents this error by avoiding the premature closing of file descriptors in os_execute_impl.

Discussion

In 4a40e57cf059011e37aa45d43f48733b8e5546b0, @bakpakin made changes to os_execute_impl to address #881. For the purposes of this PR, the relevant lines are:

https://github.com/janet-lang/janet/blob/c3f4dc0c152dd1d08fdd96e406a2847e1a3f9b40/src/core/os.c#L1143-L1163

The problem is that if new_out and new_err are the same file descriptor, calling posix_spawn_file_actions_addclose for new_out before pos_spawn_file_actions_adddup2 for new_err will cause an error when posix_spawn is later called.

Alternatives

Another possible solution to this problem would be to establish a practice of not using the same file descriptor when redirecting IO. That seems to violate the principle of least surprise and so is not the approach taken by this PR.

pyrmont commented 1 year ago

I don't understand why the 'Build and test on Windows' test failed. It didn't fail on my fork (see here).

pyrmont commented 1 year ago

I don't understand why the 'Build and test on Windows' test failed. It didn't fail on my fork (see here).

This was my mistake. The test has been fixed and CI is now passing on all platforms.

bakpakin commented 1 year ago

LGTM