ChildProcess.spawnWindows: PATH search fixes + optimizations

[squeek502]

Note: This is probably best reviewed commit-by-commit, as they could each be a separate PR if I weren't making all the changes at one time.

---

If the command is specified as something.exe, the retry will now try:

C:\some\path\something.exe
C:\some\path\something.exe.COM
C:\some\path\something.exe.EXE
C:\some\path\something.exe.BAT
... etc ...

whereas before it would only try the versions with an added extension from PATHEXT, which would cause the retry to fail on things that it should find.

---

Also, I reworked some stuff to drastically reduce the amount of allocations that occur in the FileNotFound recovery block when looking through PATH.

---

Finally, CreateProcessW can now return error.InvalidExe when appropriate, and spawnWindows has been made to handle it. When the initial windowsCreateProcess in spawnWindows fails with error.InvalidExe, it will now retry with the values from PATHEXT appended before searching the PATH.

This matches cmd.exe behavior. For example, if there is only a file named mycommand in the cwd but it is a Linux executable, then running the command mycommand will result in:

'mycommand' is not recognized as an internal or external command, operable program or batch file.

However, if there is both a mycommand (that is a Linux executable) and a mycommand.exe that is a valid Windows exe, then running the command mycommand will successfully run mycommand.exe.

---

Example that demonstrates the path search problem and shows the extent of the reduced allocations:

const std = @import("std");

pub fn main() !void {
    var diag_allocator = std.testing.FailingAllocator.init(std.heap.page_allocator, std.math.maxInt(usize));
    const allocator = diag_allocator.allocator();
    defer {
        std.debug.print("allocations: {}, allocated bytes: {}\n", .{ diag_allocator.allocations, diag_allocator.allocated_bytes });
    }

    // Note: passing env map doesn't change anything, the same behavior happens if
    //       env map isn't provided
    var env_map = try std.process.getEnvMap(allocator);
    defer env_map.deinit();

    var result = try std.ChildProcess.exec(.{
        .allocator = allocator,
        // this should be found in the system32 dir
        .argv = &[_][]const u8{"whoami.exe"},
        .env_map = &env_map,
    });
    defer allocator.free(result.stdout);
    defer allocator.free(result.stderr);

    std.debug.print("{}\n", .{result.term});
}

Before:

allocations: 819, allocated bytes: 66500
error: FileNotFound
C:\Users\Ryan\Programming\Zig\zig\lib\std\os\windows.zig:1600:32: 0x7ff7963d079b in CreateProcessW (winpath.exe.obj)
            .FILE_NOT_FOUND => return error.FileNotFound,
                               ^
C:\Users\Ryan\Programming\Zig\zig\lib\std\child_process.zig:1078:5: 0x7ff7963d0656 in windowsCreateProcess (winpath.exe.obj)
    return windows.CreateProcessW(
    ^

After:

child_process.ChildProcess.Term{ .Exited = 0 }
allocations: 129, allocated bytes: 18872

However, this is comparing the command not being found with it being found. For completeness, here's an apples-to-apples comparison for the number of allocations when the command is not found:

allocations: 127, allocated bytes: 17757
error: FileNotFound
C:\Users\Ryan\Programming\Zig\zig\lib\std\os\windows.zig:1600:32: 0x7ff6ab790ecb in CreateProcessW (winpath.exe.obj)
            .FILE_NOT_FOUND => return error.FileNotFound,
                               ^

Note also that the number/size of the allocations saved depends on the number of items within PATH and PATHEXT.

[squeek502]

Out of curiosity, I wrote a simple benchmark that just calls exec with whoami in a loop, and the allocation optimizations don't make an actual difference in runtime (presumably, since so much of the time is taken by the kernel):

Benchmark 1: winpathbench-master.exe
  Time (mean ± σ):     832.0 ms ±  17.8 ms    [User: 249.1 ms, System: 748.4 ms]
  Range (min … max):   813.6 ms … 857.4 ms    10 runs

Benchmark 2: winpathbench-pr.exe
  Time (mean ± σ):     834.7 ms ±   3.2 ms    [User: 233.4 ms, System: 728.1 ms]
  Range (min … max):   830.7 ms … 841.4 ms    10 runs

Summary
  'winpathbench-master.exe' ran
    1.00 ± 0.02 times faster than 'winpathbench-pr.exe'

Benchmark code:

const std = @import("std");

pub fn main() !void {
    const allocator = std.heap.c_allocator;
    var i: usize = 0;
    const iterations = 100;
    while (i < iterations) : (i += 1) {
        var result = try std.ChildProcess.exec(.{
            .allocator = allocator,
            .argv = &[_][]const u8{"whoami"},
        });
        defer allocator.free(result.stdout);
        defer allocator.free(result.stderr);

        if (result.term != .Exited or result.term.Exited != 0) {
            return error.CommandFailed;
        }
    }
}

[matu3ba]

Looking good so far, but this is still missing ci tests (eg. standalone tests in #13639).

For the search extensions: Are there use cases, where this is strictly necessary or is this behavior specified somewhere? Which programs are relying on these behavior?
I am wondering, because cmd has more hacks https://stackoverflow.com/a/2730717 and so has linux shell with ~ resolving to $HOME, but they are not standardized in posix api (only shell).

More a general question: Can absolute paths still lead to an evaluation of PATH and PATHEXT? Must PATHEXT also be applied on an absolute path?

[squeek502]

Will try to include tests if possible, but might do that in a follow up PR if that's acceptable.

For the search extensions: Are there use cases, where this is strictly necessary or is this behavior specified somewhere? Which programs are relying on these behavior?

I think you're right to question this, as we currently use CreateProcessW which in theory only supports executables (.exe and .com I think), and batch scripts (.bat, .cmd) are meant to be handled specially (passed as cmd.exe /c <path to batch script> <args>, see here [the part that starts "To run a batch file, ..."]). However, there seems to be undocumented support for .bat and .cmd files without cmd.exe /c in CreateProcessW, so those currently ~work too.

Other values in %PATHEXT% like .JS need extra special handling that we do not support at all. This is all outlined well in this Rust issue:

• Windows: Support running script files using process::Command rust-lang/rust#94743

So, I think the best thing to do is to parse %PATHEXT% like we do, but only actually evaluate extensions we know we support (.EXE, .COM, .BAT, .CMD) and skip the rest. I'll likely do that in this PR or a follow up.

As for documentation about how/when to use PATHEXT, I haven't been able to find that.

(for what it's worth, ReactOS doesn't seem to use the PATHEXT env var at all for its process code (it just hard codes the common extensions), but does for its where implementation)

More a general question: Can absolute paths still lead to an evaluation of PATH and PATHEXT? Must PATHEXT also be applied on an absolute path?

PATHEXT should still be used on absolute paths, but there's no reason to still search PATH. I have this change locally and will push it in a bit.

[squeek502]

This is good to go unless added tests are required before merging. I'll have a follow up PR with added tests and another that focuses solely on performance improvements.

[andrewrk]

Thanks @squeek502!