Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix windows create process retry/path search #2705

Merged
merged 1 commit into from
Jun 18, 2019

Conversation

marler8997
Copy link
Contributor

@marler8997 marler8997 commented Jun 18, 2019

Windows works a bit different than linux when it comes to searching for executables.

Windows uses the PATHEXT environment variable to iterate through a list of extensions to try when searching for executables.

For example, if you're trying to execute program foo, windows will search for foo.EXT where EXT is any one of the semi-colon separated entries in PATHEXT. So if PATHEXT is .EXE;.BAT.COM.CMD then it will search for:

  • foo.EXE
  • foo.BAT
  • foo.COM
  • foo.CMD

in that order. Note that it will check each extension in each PATH directory entry before moving to the next PATH directory entry.

I also modified the "CreateProcess" retry loop to continue the retry loop even when "access denied" errors are encountered.

Note that I discovered this issue while running my project at https://github.com/bettertools/git-extra

> cd src_zig
> zig build
> out\git-fetchout.exe origin master

} else |err| switch (err) {
error.FileNotFound => { continue; },
error.AccessDenied => { continue; },
else => { return err; },
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
else => { return err; },
else => |e| return err,

This will return an error but with the type missing the 2 handled errors above. This might not matter in this case but in general it helps with error set inference.

retry: while (it.next()) |search_path| {
var ext_it = mem.tokenize(PATHEXT, ";");
while (ext_it.next()) |app_ext| {
const app_basename = try mem.concat(self.allocator, u8, [_][]const u8{app_name[0..app_name.len - 1], app_ext});
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nothing needed to do here, but note that #265 will make this minus one unnecessary and more type-safe.

@andrewrk andrewrk merged commit c7bcf1a into ziglang:master Jun 18, 2019
@marler8997 marler8997 deleted the windowsPathSearch branch June 18, 2019 17:35
@GreyDodger
Copy link

so now when you run ChildProcess.init(...) and ChildProcess.spawnAndWait() with an app name that already contains an extension (e.g. 'aseprite.exe') that's going to break, it'll look for 'aseprite.exe.com', 'aseprite.exe.exe', etc.. is this correct?

@marler8997
Copy link
Contributor Author

If it's an actual path, it should still work. But if it's just a relative filename to a directory in PATH then yes this change breaks that case. Not sure what the right fix is. Maybe we should first check if the file exists with no extension, then go through PATHEXT?

Of course, this code in general is re-implementing the search algorithm that's already implemented inside CreateProcess. If you set lpApplicationName to NULL, CreateProcess should already perform all this search logic, the real correct solution may be to strip out all this logic and just use what Windows implements.

@marler8997
Copy link
Contributor Author

marler8997 commented Jun 27, 2019

Working on a fix here: #2770

I'm adding the "no extension" case to the while loop. It looks like windows does support this use case.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants