src/std/process.d: _spawnvp error handling is broken

[dlangBugzillaToGithub]

danny.milo (@daym) reported this on 2014-11-19T21:33:14Z

Transfered from https://issues.dlang.org/show_bug.cgi?id=13753

CC List

• bugzilla (@WalterBright)

Description

The _spawnvp in src/std/process.d is broken.

First, if the child process (say it has pid B) fails to execvp, it's a bad idea to then throw an Exception. The entire point of spawnvp in general is to hide the fact that the current process A was forked off. But now the Exception propagates through process B, so certainly the caller will notice that something is off (the caller is suddenly inside another process than he started out in). 

Later on, the waitpid result is not checked. It is possible for waitpid to return (-1). In that case, errno contains the error code and "status" contains garbage, which is then compared against.

Also, all Posix system calls can return (-1) and errno = EINTR (see <http://www.jwz.org/doc/worse-is-better.html>, search for "PC loser-ing") to indicate that while the user process asked for action S to be performed, really it should be checking and doing some other action T before.

So for the latter there really should be some global delegate that is called on EINTR which decides whether to do anything about it, possibly terminating the loop (or not, it depends). 

This is not specific to process.d but all functions that do system calls should call this. Even std.stdio.File functions should do this.

Also, it throws an Exception (literally that) using strerror_r to build it instead of just using ErrnoException. Why?

!!!There are attachements in the bugzilla issue that have not been copied over!!!

[dlangBugzillaToGithub]

danny.milo commented on 2014-11-19T21:36:22Z

Created attachment 1454
Tests whether spawnvp magically forks processes for the caller

Tests whether spawnvp magically switches processes. Try with version(Posix) implementation of spawnvp.

[dlangBugzillaToGithub]

danny.milo commented on 2014-11-19T21:56:03Z

Created attachment 1455
Patch to make _spawnvp less bad

checks waitpid() return value, does not magically put the caller into a new process. Does not properly handle EINTR.

[dlangBugzillaToGithub]

danny.milo commented on 2014-11-19T22:00:46Z

See http://www.gnu.org/software/libc/manual/html_node/Interrupted-Primitives.html

[dlangBugzillaToGithub]

danny.milo commented on 2014-11-25T18:25:13Z

Also, both the OSX and the Posix version of browse in the same file are broken in the same way...

[dlangBugzillaToGithub]

bugzilla (@WalterBright) commented on 2019-12-10T10:27:22Z

spawnvp seems only to exist for windows now. But I did not check if this has been the case in 2014 too.

Tried to adapt the test for "browse":

```
import std.process;
import core.thread;

int main() {
        auto pidBefore = getpid();
        try {
                browse("DOESNOTEXIST");
                assert(false);
        } catch(Exception e) {
                auto pidAfterwards = getpid();
                assert(pidBefore == pidAfterwards); // make sure we are still in the same process
        }
        return 0;
}
```

This produces an Assertion failure (POSIX). And the process seems still to be running after the main process has stopped.