std.fs: improve error-handling for openDirW

[yuyoyuppe]

Scenario where the bug manifests itself in a real use-case.

While adding a cmake dependency to build.zig with:

b.addSystemCommand(&[_][]const u8{ "cmake", "-DCMAKE_BUILD_TYPE=Release", ".", "-B", "build" });

I've encountered unreachable which was caused by this line. It turns out my %PATH% had an incorrect entry: :\Program Files\7-Zip - notice no C at the start.

So what happens is that as part of spawnWindows Zig searches through each %PATH% entry to find an absolute path of cmake executable, and instead of skipping an invalid entry we panic on it.

You can repro the panic with this MRE:

var buffer: [std.fs.MAX_PATH_BYTES]u8 = undefined;
var fba = std.heap.FixedBufferAllocator.init(&buffer);
const allocator = fba.allocator();
var broken_dir = try std.unicode.utf8ToUtf16LeWithNull(allocator, ":\\Program Files\\7-Zip");
_ = try std.fs.cwd().openDirW(broken_dir, .{}, true);

There's no info in the original commit why it handles it as such, so I assume it's reasonable to treat the error as error.NotDir instead.

[squeek502]

My understanding is the the unreachable here is intentional because it's assumed to be programmer error to pass invalid/malformed paths to fs functions. I'm not totally sold on that reasoning personally, but in keeping with this logic, the fix to the PATH searching problem might be to validate search paths instead of removing the unreachable here.

The relevant code for that would be within this loop:

zig/lib/std/child_process.zig

Line 1035 in 289e8fa

while (it.next()) |search_path| {

I'm unsure what validation should be done in particular--is it required for search paths to be absolute? If so, then an isAbsolute check might be sufficient. If not, then it might need some special handling.

EDIT: Seems like relative paths do/can work in PATH, so some other validation would be necessary.

[yuyoyuppe]

I've noticed a few places where OBJECT_NAME_INVALID was handled in a similar fashion, but it isn't always consistent. If there's an intent behind those unreachables, perhaps we could make those preconditions explicit with comments/asserts.

I couldn't find a function which reliably validates path, there's windowsParsePath but it accepts stuff like "::\\Program Files\\7-Zip".

[matu3ba]

I couldn't find a function which reliably validates path

Why does isAbsolute in lib/std/fs/path.zig or resolve not work? I do agree insofar as that a function isRelative would make things more consistent and isRelative should not need any allocation.

Do you want to make a PR or do you want me to create one?
https://learn.microsoft.com/en-us/dotnet/standard/io/file-path-formats

Specifically the cases \Program Files and C:Projects sounds like they could have special handling, as they look very surprising for users to handle + its quite abit boilerplate to write for Windows.

[squeek502]

Why does isAbsolute in lib/std/fs/path.zig or resolve not work?

Relative paths are allowed as search paths in PATH, so isAbsolute would fix this particular scenario but break others. std.fs.path.resolveWindows can only fail with error.OutOfMemory, so it can't be used to determine he validity of the path.

We'll likely need a function that actually validates paths (not sure if there's much precedence for this?), or abandon the OBJECT_NAME_INVALID => unreachable strategy.

[yuyoyuppe]

If you decide to do the validation with isRelative approach, please go ahead @matu3ba.

[squeek502]

@andrewrk if it makes this easier to review, this PR is a very targeted change to fix one particular manifestation (bad path in %PATH% leading to unreachable when spawning a process on Windows) of what I think is a much larger problem that I've laid out here: #15607

My personal opinion about this particular PR:

• This should only be merged if INVAL/OBJECT_NAME_INVALID in fs APIs incorrectly marked as unreachable #15607 is decided on and the OBJECT_NAME_INVALID => unreachable strategy is walked back
• In that case, though, error.NotDir is the wrong error to translate OBJECT_NAME_INVALID to, and instead a new error like error.BadPathName would likely be added to the error set for this case

[andrewrk]

I solved this in f1a9344 and the path forward on #15607 is now clear.