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.

[briansmith]

Oh, also, we should consider documenting how one would spawn test\a.com if test\a.com.exe also exists, or document that this is basically impossible.

[ChrisDenton]

Fair enough. But why does the behaviour of _wspawnlpe matter here? Also is there a reason you're only interested in .com and .bat extensions? Are you saying they should be treated specially when resolving the path?

[briansmith]

But why does the behaviour of _wspawnlpe matter here?

AFAICT, this is the closest thing a Windows C programmer has to posix_spawn. We could also check out the MinGW/Cygwin implementation of posix_spawn.

Besides the behavior of those three functions, it would also be good to know what the cmd.exe and PowerShell shells do, in these situations.

Also is there a reason you're only interested in .com and .bat extensions?

".com" files are a different kind of executable on DOS/Windows systems; roughly they are .EXEs without the PE header.

Are you saying they should be treated specially when resolving the path?

To the extent that the aforementioned APIs agree, I don't think we should do anything (by default) different.

The current behavior is surprising in the case where the input path has an extension, especially when the input path has an extension that is considered "executable," like ".com," so I think it's worth validating that the surprising behavior is what is "normally" done on Windows, by other APIs and by the shells.

I think the average programmer would expect the behavior to be "if a file exists at the input path exists, then spawn it. Otherwise, if it didn't have any extension, try appending .exe."

Consider how one would write a program where the user is given a file picker, and based on the file the user picked, that executable is run. Very much like Windows + R. It would be strange to have the user pick "debug.com" but then run a different program "debug.com.exe" that happened to exist next to it, but which the user didn't pick.

In the future it might be worthwhile to extend the Command API to add a way to disable the extension munging behavior, regardless of what the default behavior ends up being.

[ChrisDenton]

What I'm trying to tease out here is how seriously we should take files without .exe, .com or .bat extensions. Should we assume at least one of these three is meant?

The trouble I'm thinking of is that there's a mismatch between common programmer behaviour (especially those with a *nix background) and Windows conventions. On the one hand it's very common for programmers to leave off the .exe, on the other hand executables ending in .exe is a very strong convention (or .com/.bat for old command prompt scripts).

So we might end up in the opposite situation where appending .exe was intended but another file was run because it matched the exact path.

[briansmith]

What I'm trying to tease out here is how seriously we should take files without .exe, .com or .bat extensions. Should we assume at least one of these three is meant?

Regarding batch files, here's what the CreateProcessW docs (https://docs.microsoft.com/en-us/windows/win32/api/processthreadsapi/nf-processthreadsapi-createprocessw) say:

To run a batch file, you must start the command interpreter; set lpApplicationName to cmd.exe and set lpCommandLine to the following arguments: /c plus the name of the batch file.

So, based on those docs, I wouldn't expect "foo.bat" to be treated specially.

Regarding .com files, this is what the CreateProcessW documentation says:

Therefore, if the file name extension is .com, this parameter must include the .com extension.

That implies to me that specifying "foo.com" should never result in "foo.com.exe".

In fact, you seem to be saying that the CreateProcessW documentation is wrong or missing important details. Perhaps we should file a pull request against https://github.com/MicrosoftDocs/sdk-api/blob/docs/sdk-api-src/content/processthreadsapi/nf-processthreadsapi-createprocessw.md to get help from Microsoft on correcting the docs or our understanding of them.

[ChrisDenton]

The docs are describing the behaviour when searching PATH. They do not describe the behaviour when using an absolute path or a relative path that isn't a plain file name. This is essentially undocumented. The docs could indeed be updated.

But that's beside the point I was trying to make here. Current behaviour aside, I'm asking whether when someone does Command::new(r"Z:\test\app-1.23") would it be surprising that app-1.23 is tried before app-1.23.exe? And would this in fact be a concern? Or is it important that all extensions are treated equally regardless of if they are on the "executable" list?

[ChrisDenton]

Ok, I had time to run some tests on CreateFileW. The following gives the search order when appending Filename to the given path (e.g. .\test\a, test\a).

Filename	.\test\	test\
a	a, a.exe	a.exe
a.exe	a.exe, a.exe.exe	a.exe
a.bat	a.bat, a.bat.exe	a.bat
a.txt	a.txt, a.txt.exe	a.txt
a.com	a.com, a.com.exe	a.com

In summary, a .\test path will try the exact name first then try appending .exe. Whereas a test\ path will try the exact name and only append .exe if there's no extension at all (without trying the exact name at all). Thus the results depend on the particular type of path used.

I see some possibilities for the Rust standard library:

1. Exactly replicate the above behaviour. This would mean testing the relative path for any . or .. components and adjusting the behaviour accordingly.
2. Only add a .exe extension if there is no extension. May break code relying on the .\ behaviour.
3. Switch the test order so the exact name is always tried before appending .exe.
4. Keep the current behaviour but special case filenames with .com and .bat extensions.

[briansmith]

The docs are describing the behaviour when searching PATH. They do not describe the behaviour when using an absolute path or a relative path that isn't a plain file name. This is essentially undocumented. The docs could indeed be updated.

Here's what the docs say at https://docs.microsoft.com/en-us/windows/win32/api/processthreadsapi/nf-processthreadsapi-createprocessw:

If the file name does not contain an extension, .exe is appended. Therefore, if the file name extension is .com, this parameter must include the .com extension. If the file name ends in a period (.) with no extension, or if the file name contains a path, .exe is not appended. If the file name does not contain a directory path, the system searches for the executable file in the following sequence:

Further, as you've noted, CreateProcessW has a mechanism, lpApplicationName, to disable the extension munging completely.

But that's beside the point I was trying to make here. Current behaviour aside, I'm asking whether when someone does Command::new(r"Z:\test\app-1.23") would it be surprising that app-1.23 is tried before app-1.23.exe?

It would be surprising if it works differently than CreateProcessW in any way not documented in the Command documentation.

And would this in fact be a concern? Or is it important that all extensions are treated equally regardless of if they are on the "executable" list?

The "executable list" is, AFAICT, the value of $PathExt, but maybe that's only supposed to be used by ShellExecuteEx; I don't know if CreateProcessW uses $PathExt; that would be something interesting to learn.

[ChrisDenton]

The "executable list" is, AFAICT, the value of $PathExt, but maybe that's only supposed to be used by ShellExecuteEx; I don't know if CreateProcessW uses $PathExt; that would be something interesting to learn.

I does not. The only special extensions are:

• .exe (which it may or may not append to a filename).
• Testing for .com and .bat which, if present, causes it to rewrite the command such that cmd.exe is called instead and the given path is passed as an argument. EDIT: This behaviour is undocumented.

[ChrisDenton]

I feel like we're getting a bit lost in the weeds there. So I'll try to sum up where I'm at as best I can:

If you're saying we should behave exactly as CreateProcessW behaves then this is very actionable and I would entirely support changes to that effect.

If you're saying we only need to behave how CreateProcessW is documented to behave, then there are questions about how we should treat changing undocumented behaviour.

[briansmith]

Ok, I had time to run some tests on CreateFileW.

I think you mean CreateProcessW?

The following gives the search order when appending Filename to the given path (e.g. .\test\a, test\a).

Filename .\test\ test\
a a, a.exe a.exe
a.exe a.exe, a.exe.exe a.exe
a.bat a.bat, a.bat.exe a.bat
a.txt a.txt, a.txt.exe a.txt
a.com a.com, a.com.exe a.com

The a.exe line is especially surprising. It's worth double-checking that. Maybe you could post your test program somewhere so people could look at it.

If you're saying we should behave exactly as CreateProcessW behaves then this is very actionable and I would entirely support changes to that effect.

If you're saying we only need to behave how CreateProcessW is documented to behave, then there are questions about how we should treat changing undocumented behaviour.

It is possible that the CreateProcessW documentation is documenting the least common denominator behavior across all versions of Windows that are currently supported, or that it is out of date (which is maybe the same thing, if the additional helpful behaviors were implemented in later versions of Windows). (Which versions of Windows have you run your test programs on?)

My concern is that it may be unrealistic to "Do what CreateProcessW does, except ignore the current working directory, and append .exe when we need to but never append .exe when we shouldn't," when we consider the differences across versions of Windows, and maybe even if we don't consider the differences across versions of Windows.

[briansmith]

I feel like we're getting a bit lost in the weeds there.

I agree. Here are my main concerns:

• Does the "does the file exist" check we do in resolve_exe create a race condition when combined with file existence checks that CreateProcessW will do. To resolve this, we just need to add a comment such as "Passing lpApplicationName to CreateProcessW is essential for preventing race conditions with respect to the file existence check(s) done in resolve_exe and similar checks that would be done in CreateProcessW if lpApplicationName were NULL."
• If "foo.com" and "foo.com.exe" both exist in the same folder, sometimes `Command::spawn() will execute "foo.com.exe" when I ask it to execute "foo.com". The same for ".cmd" and ".bat". The current behavior is, at a minimum, surprising, and at least it should be documented, if not changed to be more consistent with what CreateProcessW does.
• There should be a way to tell Command to disable it's extension fixing logic so that the "I asked the user to pick a file to run and I want to run exactly that file" use case can be supported (just like CreateProcessW with lpApplicationName).

Here are some secondary concerns:

• I feel like the code is under-documented. One really has to read GitHub discussions to understand why it is doing what it is doing.
• As I mentioned above, some of the existing comments that do exist don't match the existing code.
• Maybe I'm looking in the wrong spot, but I feel like we're missing a bunch of essential tests. Maybe the code needs to be refactored a bit to make it practical to write unit tests for resolve_exe. These tests would go a long way towards clarifying the behavior and making security review easier.
• Issue Windows: Reduce raciness of spawn() vs. SetCurrentDirectory #93133.