Windows: Audit, clarify, and add tests for `resolve_exe()` "append .EXE extension if the extension is different" (not just missing) behavior.

[briansmith]

I was reading the code for resolve_exe() for some reason and I saw this code:

    let has_exe_suffix = if exe_path.len() >= EXE_SUFFIX.len() {
        exe_path.bytes()[exe_path.len() - EXE_SUFFIX.len()..]
            .eq_ignore_ascii_case(EXE_SUFFIX.as_bytes())
    } else {
        false
    };

(Incidentally, can't this be simplified?)

Then later:

        if has_exe_suffix {
             return [...];
        };

        // Append `.exe` if not already there.
        path = path::append_suffix(path, EXE_SUFFIX.as_ref());
        if path.try_exists().unwrap_or(false) {
            return Ok(path);
        } else {
            // It's ok to use `set_extension` here because the intent is to
            // remove the extension that was just added.
            path.set_extension("");
            return Ok(path);
        }

Appending EXE_SUFFIX if the extension isn't already EXE seems wrong to me; I think instead it would make more sense to use exactly the same rules that CreateProcessW is documented to use, which is roughly "there is no extension," not "the extension isn't .exe".

Further, let's say there is an extension that isn't EXE_SUFFIX, and we append EXE_SUFFIX and we find that the file doesn't exist. Then we remove the extension. The comment implies that this will reset the extension to what it was before, but this is only true if there was no extension at all. What if there was an extension that wasn't EXE_SUFFIX?

[ChrisDenton]

(Incidentally, can't this be simplified?)

I for one would more than welcome any PRs helping to clean up this code! Though I think at this point it might be worth having a WindowsPath type that can handle things like this, even if it's only internal to the std.

Appending EXE_SUFFIX if the extension isn't already EXE seems wrong to me; I think instead it would make more sense to use exactly the same rules that CreateProcessW is documented to use, which is roughly "there is no extension," not "the extension isn't .exe".

You're right that this is weird but the goal of the code was to be as close to the actual behaviour of CreateProcessW as possible. How the extension is or isn't added depends on the type of path given. Lower down you'll see that when just a file name is given, an extension is added only if there isn't one already (which is what CreateProcessW is documenting).

If you can make some test cases that work in Rust 1.57 but not 1.58 then that'd help to find any issues. I mean, we could simplify things here regardless but I'd personally want to be cautious about potentially breaking people's code.

Further, let's say there is an extension that isn't EXE_SUFFIX, and we append EXE_SUFFIX and we find that the file doesn't exist. Then we remove the extension. The comment implies that this will reset the extension to what it was before, but this is only true if there was no extension at all. What if there was an extension that wasn't EXE_SUFFIX?

use std::path::PathBuf;
fn main() {
    let mut p = PathBuf::from("test.txt.exe");
    p.set_extension("");
    println!("{}", p.display());
}

Output:

test.txt

@rustbot label +O-windows

[briansmith]

Appending EXE_SUFFIX if the extension isn't already EXE seems wrong to me; I think instead it would make more sense to use exactly the same rules that CreateProcessW is documented to use, which is roughly "there is no extension," not "the extension isn't .exe".

You're right that this is weird but the goal of the code was to be as close to the actual behaviour of CreateProcessW as possible. How the extension is or isn't added depends on the type of path given. Lower down you'll see that when just a file name is given, an extension is added only if there isn't one already (which is what CreateProcessW is documenting).

Do you mean that the documented behavior of CreateProcessW at https://docs.microsoft.com/en-us/windows/win32/api/processthreadsapi/nf-processthreadsapi-createprocessw doesn't match the actual behavior, and we're intentionally implementing the actual behavior and not the documented behavior? Or do you think the current behavior is consistent with the documented behavior? My reading, which is maybe a misunderstanding, is that CreateProcessW won't append EXE_SUFFIX if the file already has any extension.

A particular interesting test case input would be "test.com". We wouldn't want to modify it to "test.com.exe".

Thanks for clarifying the set_extension("") bit for me. TIL I didn't understand what that function does.

[ChrisDenton]

I mean CreateProcessW is documenting one of its behaviours but not the other one. When searching the PATH using only a filename it does as is documented. When using a full path or a relative path the behaviour is undocumented and was replicated in this code from testing.

I shall look back at my test cases and see if I missed anything.

[briansmith]

When using a full path or a relative path the behaviour is undocumented and was replicated in this code from testing.

Another thing to consider is whether path.set_extension("") maybe should instead be path.set_extension("."); or something else that will stop CreateProcessW from implmeneting its "maybe append .exe" logic. Otherwise, there would be a race:

• path + ".exe" doesn't exist at the time resolve_exe checks to see that it exists, so it returns path.
• path + ".exe" is created before spawn() calls CreateProcessW.
• CreateProcessW looks for the .exe version and finds it, and so executes it, whereas we expected it to execute the file without .exe appended to it.

[ChrisDenton]

We now use the lpApplicationName parameter to pass the path. The special behaviour only applies when using NULL for lpApplicationName.

[briansmith]

We now use the lpApplicationName parameter to pass the path. The special behaviour only applies when using NULL for lpApplicationName.

I think it would be good to add a comment at

rust/library/std/src/sys/windows/process.rs

Line 312 in 181e915

let program = to_u16s(&program)?;

saying that we must pass a non-NULL lpApplicationName to prevent this kind of race.

Also at

rust/library/std/src/sys/windows/process.rs

Lines 363 to 364 in 181e915

	// It falls back to the plain file name if a full path is given and the extension is omitted
	// or if only a file name is given and it already contains an extension.

We should update the comments to more accurately describe the behavior:

• The current comment says "full path," but It isn't just for "full paths," but for any path that isn't a plain filename.
• The current comment says "the extension is omitted," but the condition is actually "the extension isn't EXE_SUFFIX" (though, this seems wrong to me, as I mentioned in the initial comment above).

[ChrisDenton]

saying that we must pass a non-NULL lpApplicationName to prevent this kind of race.

Makes sense. Though note that to_u16s(&program)?; ensures we will never pass NULL.

We should update the comments to more accurately describe the behavior

This is true. Though I'd ideally want to confirm the desired behaviour first so code and comment can be updated at the same time.

[ChrisDenton]

Ok I've done a quick test using the pre-resolver Rust (1.57). I created a hello world type program and saved it as Z:\test\hello.txt.exe.

I then tried to run it as hello.txt using the following paths (with Z:\ as the current directory):

path	result
Z:\test\hello.txt	found
.\test\hello.txt	found
test\hello.txt	not found

The last case is an added complexity I hadn't accounted for.

[briansmith]

I question whether anybody would want spawn("Z:\test\hello.txt") to run C:\test\hello.txt.exe, regardless of what the previous behavior of rust libstd was. The rationale for appending ".exe" is that many Rust programs expect to spawn executables that usually don't have an extension (e.g. on Unix) but have an ".exe" extension on Windows.

Also, consider: test\hello.com and test\hello.com.exe, both executables in the same directory

[briansmith]

Also, test\hello.bat and test\hello.bat.exe. I think CreateProcessW won't run a batch file, but also anybody trying to spawn "hello.bat" isn't trying to spawn "hello.bat.exe".

[ChrisDenton]

CreateProcessW will run a batch file. Originally the new resolver broke this undocumented behaviour but this was fixed.

In general the rationale for matching the old behaviour was to avoid inadvertently breaking applications in an unrelated PR. And it is possible someone may want to run exes such as app-1.23.exe or app.new.exe. I think if new resolver behaviour is desired then that should be a new issue and a separate PR so as to avoid conflating issues.

I would be happy to make such breaking changes if they're desirable but they should be done intentionally, not as a side-effect. Obviously I failed somewhat in exactly matching the edge cases of CreateProcessW, though I seem to have gotten away with it so far which suggests we might have some wiggle room to further adjust the resolver.

[briansmith]

CreateProcessW will run a batch file. Originally the new resolver broke this undocumented behaviour but this was fixed.

I see.

I think there are two issues:

• Is the new version of Command::spawn doing the same thing as the old one, in this respect?
• Was the old/new version of Command::spawn doing the right thing?

I think we'd need to collect more info:

• If test\a.com doesn't exist and test\a.com.exe exists, will X("test\a.bat") run a.bat.exe?
• If test\a.com and test\a.com.exe both exist, which one will X("test\a.bat") run?
• If test\a.bat and test\a.bat.exe both exist, which one will X("test\a.bat") run?
• If test\a.bat doesn't exist and test\a.bat.exe exists, will X("test\a.bat") run a.bat.exe?

...for all X in { CreateProssessW, _wspawnlpe, Command::spawn }.

I think for any case where all three of these functions agree, then there's nothing to consider changing.