cbits: Ensure that fork pipe doesn't shadow std fds #280
Conversation
|
Cc @blackgnezdo. It would be very much appreciated if you could test this. |
Previously we would assume that the pipe used to communicate errors from the child back to the parent did not shadow the standard descriptors (stdin, stdout, and stderr). Somewhat surprisingly, this appears to hold most platforms. However, OpenBSD appears to be a notable exception. This lead to the failure of the `processT251` test. Avoid relying on this assumption by `dup`ing the pipe fds until they end up out of the standard fd range. Closes haskell#266.
| */ | ||
| int unshadow_pipe_fd(int fd, char **failed_doing) { | ||
| if (fd <= 2) { | ||
| int fd2 = dup(fd); |
There was a problem hiding this comment.
I tested this, it seems to not be doing what's expected. The trace looks like this:
54688 processT251 STRU int [2] { 0, 3 }
54688 processT251 RET pipe 0
...
54688 processT251 CALL dup(0)
54688 processT251 RET dup 4
...
54688 processT251 CALL close(4)
54688 processT251 RET close 0
The recursive call below returns the same same 4 that it was passed in (i.e. both fd3=fd2=4) which then gets closed before it's returned, so the caller gets an invalid fd and the test reports:
$ ./processT251
parent start
child start
processT251: ./processT251: createProcess: read pipe: invalid argument (Bad file descriptor)
processT251: user error (Pattern match failure in 'do' block at processT251.hs:20:5-15)
child2 start
child2 done
There was a problem hiding this comment.
I'm testing this now:
diff --git a/cbits/posix/fork_exec.c b/cbits/posix/fork_exec.c
index 9c98446..a613763 100644
--- a/cbits/posix/fork_exec.c
+++ b/cbits/posix/fork_exec.c
@@ -108,24 +108,23 @@ setup_std_handle_fork(int fd,
* errors. See #266.
*/
int unshadow_pipe_fd(int fd, char **failed_doing) {
- if (fd <= 2) {
- int fd2 = dup(fd);
- if (fd2 == -1) {
- *failed_doing = "dup(unshadow)";
- return -1;
- }
-
- // This should recurse at most three times
- int fd3 = unshadow_pipe_fd(fd2, failed_doing);
- if (close(fd2) == -1) {
- *failed_doing = "close(unshadow)";
- return -1;
- }
-
- return fd3;
- } else {
- return fd;
- }
+ int i = 0;
+ int fds[3] = {0};
+ for (i = 0; fd < 3 && i < 3; ++i) {
+ fds[i] = fd;
+ fd = dup(fd);
+ if (fd == -1) {
+ *failed_doing = "dup(unshadow)";
+ return -1;
+ }
+ }
+ for (int j = 0; j < i; ++j) {
+ if (close(fds[j]) == -1) {
+ *failed_doing = "close(unshadow)";
+ return -1;
+ }
+ }
+ return fd;
}
/* Try spawning with fork. */There was a problem hiding this comment.
I kicked the tires with a hand-written test program and it works. For some reason I can't make the test pass when running from within the ghc test suite. The trace looks different though after my change
19355 processT251 STRU int [2] { 0, 3 }
19355 processT251 RET pipe 0
19355 processT251 CALL dup(0)
19355 processT251 RET dup 4
19355 processT251 CALL close(0) <- closes the "bad" descriptor now
19355 processT251 RET close 0
19355 processT251 CALL vfork()
90064 processT251 RET vfork 0
90064 processT251 CALL close(4)
90064 processT251 RET close 0
90064 processT251 CALL fcntl(3,F_SETFD,FD_CLOEXEC)
90064 processT251 RET fcntl 0
90064 processT251 CALL sigprocmask(SIG_SETMASK,0<>)
90064 processT251 RET sigprocmask 0x2<SIGINT>
90064 processT251 CALL close(0)
90064 processT251 RET close -1 errno 9 Bad file descriptor
Looks like something else is broken. I'd debug it, but I need to figure out how to build the test program with a modified process library without rebuilding all of ghc.
There was a problem hiding this comment.
I stared at this a bit more. I attribute the failure of close above to
setup_std_handle_fork(STDIN_FILENO, stdInHdl, forkCommunicationFds[1]);
which tries to close the fd which is already closed. The test in question finally passes when I make close failure non-fatal:
diff --git a/cbits/posix/fork_exec.c b/cbits/posix/fork_exec.c
index a613763..82cc7af 100644
--- a/cbits/posix/fork_exec.c
+++ b/cbits/posix/fork_exec.c
@@ -65,7 +65,7 @@ setup_std_handle_fork(int fd,
{
switch (b->behavior) {
case STD_HANDLE_CLOSE:
- if (close(fd) == -1) {
+ if (close(fd) == -1 && errno != EBADF) {
child_failed(pipe, "close");
}
return 0;
|
This has been superceded by #282. |
2 participants
Previously we would assume that the pipe used to communicate errors from the child back to the parent did not shadow the standard descriptors (stdin, stdout, and stderr). Somewhat surprisingly, this appears to hold most platforms. However, OpenBSD appears to be a notable exception.
Avoid relying on this assumption by
duping the pipe fds until they end up out of the standard fd range.Closes #266.