Track errors that may happen in forked process on Posix #5431
Conversation
|
I'll try to improve coverage, though it's not easy. Code of fork does not get into coverage because it's different process. Anyway, I provided unittest with empty executable file. |
std/process.d
Outdated
| string errorMsg; | ||
| readExecResult = read(forkPipe[0], &error, error.sizeof); | ||
|
|
||
| switch(status) |
There was a problem hiding this comment.
Because noerror is not error.
There was a problem hiding this comment.
Then add it with assert(false) as the case body.
There was a problem hiding this comment.
Also, space after switch (CC @wilzbach)
std/process.d
Outdated
| sigaction_t ignoreAction; | ||
| memset(&ignoreAction, 0, sigaction_t.sizeof); | ||
| ignoreAction.sa_handler = SIG_IGN; | ||
| sigaction(SIGPIPE, &ignoreAction, null); |
There was a problem hiding this comment.
man sigaction says:
During an execve(2), the dispositions of handled signals are reset to the default; the dispositions of ignored signals are left unchanged.
It looks like this will cause SIGPIPE to be ignored in the child process too, then?
There was a problem hiding this comment.
You're right.
Actually I'm not sure if ignoring pipe errors is needed at all. Should we care about the case when parent process dies before forked child proceeds to execve?
There was a problem hiding this comment.
Should we care about the case when parent process dies before forked child proceeds to execve?
I can't think of a reasonable situation where it would make a difference, so I don't think so. If the parent process is gone, the child process will be disowned, in which case its exit code doesn't matter. Furthermore, unless the termination of the parent process is somehow linked with the spawnProcess call, it would be a random event, in which case it wouldn't matter whether the parent process was terminated before the fork call, or after it but before it received the child status. And if we're going to worry about either process being randomly terminated, then that also raises the question of how to handle the child process being killed before it communicates its status... not something we should worry about, I think.
|
I also added unittest for the case when user does not have search permission on working directory. This improves coverage a little by including chdir error case. |
std/process.d
Outdated
| @@ -570,7 +559,7 @@ private Pid spawnProcessImpl(in char[][] args, | |||
| string errorMsg; | |||
| readExecResult = read(forkPipe[0], &error, error.sizeof); | |||
|
|
|||
| switch(status) | |||
| switch (status) | |||
There was a problem hiding this comment.
I probably misunderstood you first. Do you want assert(0) on noerror case instead of default case?
There was a problem hiding this comment.
Yes. If almost all enum members are present in the switch and it makes sense to account for them explicitly, then use final switch with an assert(false) case body for cases which logically cannot be expected to occur for that switch expression in that point in the program.
| @@ -1081,6 +1081,17 @@ version (Posix) @system unittest | |||
| std.file.write(directory, "foo"); | |||
| scope(exit) remove(directory); | |||
| assertThrown!ProcessException(spawnProcess([prog.path], null, Config.none, directory)); | |||
|
|
|||
| // can't run in directory if user does not have search permission on this directory | |||
There was a problem hiding this comment.
Traversal (+x), not search (+r)
There was a problem hiding this comment.
I'm not sure here. What does "search permission" mean then in chdir documentation?
If it's read permission, then there should be no such test, since it relies on non-standard behavior.
There was a problem hiding this comment.
From chmod docs:
S_IXUSR (00100)
execute/search by owner ("search" applies for directories, and means that entries within the directory can be accessed)
|
I'm not sure what codecov complains about. |
|
Auto-merge toggled on |
|
Thanks for this improvement! |
This commit changes behavior of spawnProcess on Posix.
Previously spawnProcess just reported child errors via perror. But the true D way is to throw exception. The problem is that it's not easy to transfer error from forked process to parent. This commit uses pipes to solve this problem.
This commit also adds new overload for ProcessException.newFromErrno that allows to create exception object from arbitrary error number, not just from the current value of errno.
To check that it works:
Then use spawnProcess on emptyfile. The old phobos will just report error to stderr. The patched one will throw an exception with message saying that exec format is invalid.