assert_cmd icon indicating copy to clipboard operation
assert_cmd copied to clipboard

.timeout() functionality does not work on payload processes with children

Open mrkmndz opened this issue 2 years ago • 3 comments

The implementation of timeout implemented in #92 does not work as intended for payload processes with children.

Namely, it will get stuck here:

        let stdout = stdout
            .and_then(|t| t.join().unwrap().ok())
            .unwrap_or_default();

waiting for the stdout to EOF, while the "grandchild" process happily keeps that fd open indefinitely.

In order to behave as expected, we need to explicitly kill all transitive children of the payload process here:

                .unwrap_or_else(|| {
                    let _ = child.kill();
                    child.wait()
                })

or at least apply some sort of timeout logic on the read threads.

cc @epage

mrkmndz avatar Mar 23 '23 03:03 mrkmndz

What OS is this on and can you create a reproduction case to add to the tests?

When the child process is killed, I would expect the OS to close the stdio which would cause the reader threads to end

epage avatar Mar 23 '23 12:03 epage

The file descriptor will be closed if there are no more processes attached to it, but if the initial payload process spawns a child process before being killed, and that child process inherits the standard out of the payload process, then after the kill there will still be a process attached to the stdout fd, so it will not be closed.

I can try to put up a test for this, but I probably won't get to it for a while.

mrkmndz avatar Mar 23 '23 16:03 mrkmndz

If we cancel the read threads, what would be keeping us from leaking the child processes? I feel like the main issue here is that those aren't being killed and not us continuing to read from them.

epage avatar Mar 24 '23 17:03 epage