zig icon indicating copy to clipboard operation
zig copied to clipboard

change std.ChildProcess to accept a directory handle for cwd rather than a string

Open andrewrk opened this issue 5 years ago • 3 comments

In general, the standard library is intended to strongly encourage the use of open directory handles rather than path string manipulation, and this includes child process creation.

Here's my current use case:

test "foo" {
    const tmp = std.testing.tmpDir();
    defer tmp.cleanup();

    // ...

    var result = try link.updateExecutableFilePath(allocator, analyzed_module, tmp.dir, "a.out");
    defer result.deinit(allocator);

    const exec_result = try std.ChildProcess.exec(.{
        .cwd = tmp.dir,
        .argv = &[_]const u8{"./a.out"},
    });
}

andrewrk avatar Apr 27 '20 20:04 andrewrk

Re: O_CLOEXEC, from https://github.com/ziglang/zig/pull/5192#discussion_r418889666 :

Take a situation where 2 threads simultaneously spawn a child process: you don't want one messing with fds to affect the other ==> you need to do all the fd manipulation in the child (after fork; before exec). posix_spawn was invented to solve this problem (as its very hard to do correctly with respect to signals).

daurnimator avatar May 02 '20 04:05 daurnimator

I'd like to work on this, but without a pile of merge conflicts due to #14152.

matu3ba avatar Feb 15 '23 21:02 matu3ba

Take a situation where 2 threads simultaneously spawn a child process: you don't want one messing with fds to affect the other ==> you need to do all the fd manipulation in the child (after fork; before exec). posix_spawn was invented to solve this problem (as its very hard to do correctly with respect to signals).

Unfortunately it only mitigates the problem, as "directory handles designed for 1 child process" are leaked to other child processes (CLOEXEC closes them in execv[e] making them unusable in the child process), so to handle all cases without bad behavior one either

    1. uses a full spawn mutex to prevent all parallel process spawns and closes + reopens handles requiring full cooperation of complete codebase and fine-grained process handle tracking, (Rust only provides the mutex here and users report bunch of issues regarding leaked handles)
    1. close all fds to prevent leaking file handles / does not open them in the first place (more like Zig's solution).

And this does take into the account potentially misbehaving programs doing weird stuff like unsetting CLOEXEC or only using clone/fork.

Windows provides a relative convenient api to provide the to be inherited file handles per process (which is used but not exposed to user yet), but the more advanced one which includes arbitrary handles (named pipes for example) is very aweful (multiple syscalls, likely changing between versions).

tldr; External code may spawn and leak our handles. If there is no external code to integrate with, it's a matter of design. Zig should at least strive to outline that lib code should never do process spawns unless self-contained.

matu3ba avatar Jun 11 '23 22:06 matu3ba