win/spawn: run executables with no file extension

[KyleFromKitware]

This was prompted by https://gitlab.kitware.com/cmake/cmake/-/issues/25450#note_1451473.

[KyleFromKitware]

@vtjnash I'm not able to reproduce this issue locally, could you please take a look?

I think I figured it out. The build and test for mingw were happening in two different steps, and uv_run_tests{,_a}.exe was being copied, but not uv_run_tests{,_a}_no_ext. We'll see if it passes now.

Edit: it passed :)

[vtjnash]

Actually, checking this behavior on a Windows system in cmd, it is actually not permitted for a program to run without an extension. Even something explicitly given as a literal absolute path will fail to run from cmd if the file does not have an extension. It doesn't matter what the extension is, but cmd will fail to run it if it does not have one. This is not a security feature of any sort, since it doesn't have this mis-feature when there is a .; it is just a weird unnecessary edge case of the OS. Powershell is even worse, since it appears to use ShellExecute, which (despite the name) does not actually execute it but rather takes the default action in the registry for that file type (as if double-clicking on it). Sigh, I hate Windows. :/

I think we can break from cmd's inane behavior here. But might be good to get at least one other reviewer to confirm, and perhaps we should run nodejs's testsuite to make sure it isn't somehow explicitly relying on this.

[KyleFromKitware]

@vtjnash Just so you know, I have recently accepted a position at NVIDIA and will be leaving Kitware. My last day is December 19th.

In the event that this PR is not merged before then, @bradking is going to take over working on it.

[vtjnash]

@bnoordhuis are you okay with this? I am surprised that libuv cannot run a program specified literally, unless the filename includes a .. The algorithm here previously matched the cmd behavior (which would run instead a file of the same name with an added .exe extension in the same directory when given an exact name). But I just find that confusing.

[bnoordhuis]

This should maybe be a UV_PROCESS_NAME_TBD flag because:

a) it's contrary to how CreateProcess() works and therefore contrary to how users may reasonably expect it to work, and

b) it's potentially a backwards-incompatible change in behavior

[bnoordhuis]

Suggestion: move this into the else block below. That's now the the only place that uses it.

[bnoordhuis]

sizeof(char) is always 1 and use snprintf() instead of strcpy():

Suggested change

	memcpy(new_exepath, exepath, exepath_size - (sizeof(".exe") - sizeof(char)));
	strcpy(new_exepath + exepath_size - (sizeof(".exe") / sizeof(char) - 1), "_no_ext");
	snprintf(new_exepath, sizeof(new_exepath), "%.*s_no_ext",
	(int) (exepath_size - sizeof(".exe") + 1),
	exepath);

Also in the next test.

[vtjnash]

This should maybe be a UV_PROCESS_NAME_TBD flag because:

a) it's contrary to how CreateProcess() works and therefore contrary to how users may reasonably expect it to work, and

Do you mean cmd here? This change is just forwarding the user's input to CreateProcess, rather than forbidding calling a literal path if the filename does not include a .

b) it's potentially a backwards-incompatible change in behavior

It does change behavior if the user has both foo and foo.exe in a folder and tries to run ./foo expecting ./foo.exe to run. But is that necessarily breaking? It was already true that if you had both ./fo.o and ./fo.o.exe then ./fo.o would run in preference to ./fo.o.exe, though ./fo.o.exe would have run if it was not present.

[bnoordhuis]

Do you mean cmd here?

No, CreateProcess().

But is that necessarily breaking?

I'll have to get my crystal ball to answer that. :-)

You can decide if you want to merge this as-is or behind a flag.

[vtjnash]

No, CreateProcess().

Oh, I see what you mean--if you skip the first argument then CreateProcess can do a search. But in that cause, the current behavior is contradictory to the behavior of CreateProcess still, since libuv always adds .exe if there is no . and CreateProcess never adds .exe, so this PR would make them behave more similarly. See example 3 here: https://stackoverflow.com/questions/43139364/createprocess-weird-behavior-with-files-without-extension

From the docs: "If the file name... contains a path, .exe is not appended"

[bradking]

@vtjnash as Kyle mentioned in #4241 (comment) I'm taking over this work. I've revised the commit to address above feedback and pushed a branch with the same name to my own fork. You can fetch it from there and (with maintainer access) force-push it to Kyle's fork to update this PR, or just close this and I'll open a new PR. Which do you prefer?

[vtjnash]

Probably easier if you make a new PR, so that you have full control of it and don't need access rights to Kyle's repo to make any tweaks to it. I just took a quick look and it sounded good to me.

I am just wondering if we should make the new behavior the default (more closely matching unix/CreateProcess), but give users the option of explicitly asking libuv to exactly match the undocumented cmd behavior for backwards compatibility, as mentioned by Ben. Or just assume that since the fix on the user's side would be pretty easily forwards compatible (e.g. explicitly specify the .exe filename exactly, if they need to have both in the same folder and are specifying a relative path to it).

I guess the case this change could break someone's workflow (trying to guess from my opaque crystal ball), is if someone relies on having the unix script and windows exe in the same folder, and having libuv pick the right one based just on the file extension, like so:

$ ls
script
script.exe

$ cat script
#!/bin/sh
exec wine $(dirname $0)/script.exe "$@"

However, this feels distinctly non-portable already, from the assumption that the user has wine, or that the script file can run on every non-windows platform.

[bradking]

@vtjnash I opened #4292 to continue work. This PR can be closed.