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

Unix.create_process_env "bash" [| |] would use C:\Windows\System32\bash.exe even the path set #7662

Closed
vicuna opened this issue Oct 27, 2017 · 9 comments

Comments

@vicuna
Copy link

vicuna commented Oct 27, 2017

Original bug ID: 7662
Reporter: clouds56
Assigned to: @dra27
Status: acknowledged (set by @dra27 on 2017-10-28T09:48:29Z)
Resolution: open
Priority: normal
Severity: minor
Platform: win32
OS: Windows 10
OS Version: 1703
Version: 4.02.3
Category: otherlibs

Bug description

the caml_search_exe_in_path in win_create_process_native called

retcode = SearchPath(NULL,              /* use system search path */
                     name,
                     L".exe",            /* add .exe extension if needed */
                     fullnamelen,
                     fullname,
                     &filepart);

to get fullpath of "bash.exe".
while according to the remarks in https://msdn.microsoft.com/en-us/library/windows/desktop/aa365527(v=vs.85).aspx SearchPath would return executable in SystemPath when SafeProcessSearchMode is on, we should run SetSearchPathMode before search.

Steps to reproduce

  1. add C:\msys64\usr\bin to System PATH before C:\Windows\System32

  2. run in ocaml:
    Unix.create_process_env
    "bash"
    [| |]
    (Unix.environment ())
    Unix.stdin Unix.stdout Unix.stderr;;

  3. it would return: (the output is the same as C:\Windows\System32\bash.exe)
    -- Beta feature --
    This will install Ubuntu on Windows, distributed by Canonical
    and licensed under its terms available here:
    https://aka.ms/uowterms

In order to use this feature you must have Developer Mode enabled.
Press any key to continue...

Additional information

When run "opam install merlin", in windows 10. it would call Unix.make_process_env, which would return system "bash.exe" instead of the one in PATH.
The issue blocks install almost all packages with opam.

@vicuna
Copy link
Author

vicuna commented Oct 28, 2017

Comment author: @dra27

I can confirm this issue, although I don't believe this has anything to do with SafeProcessSearchMode.

As far as I can tell, it looks more like the SearchPath function behaves in the same was as CreateProcess (https://msdn.microsoft.com/en-us/library/windows/desktop/ms682425.aspx), modulo the SafeProcessSearchMode setting.

For example, if you copy C:\msys64\usr\bin\bash.exe to C:\msys64\usr\bin\regedit.exe and try to run regedit.exe (from Start -> Run) you still get the Windows Registry Editor.

My best guess is that SearchPath's definition of "system path" refers to the more detailed explanation in CreateProcess.

I'm not sure what this means for Unix.create_process_env.

@xavierleroy - my instinct is that we should be obeying PATH, not these rules, and that therefore caml_search_exe_in_path should be updated not to use SearchPath? I've already implemented similar in opam, so we have the code, it's just a matter of deciding to make what is nominally a breaking change and also replacing a quite important system call with custom code.

@vicuna
Copy link
Author

vicuna commented Oct 30, 2017

Comment author: @xavierleroy

I can confirm this issue, although I don't believe this has anything to do with SafeProcessSearchMode.

This use of SearchPath within Unix.create_process goes back 16 years, perhaps 18 years. So, if the problem shows up only now, I guess it has to do with a recent change of semantics of SearchPath, perhaps related to SafeProcessSearchMode and its default value.

my instinct is that we should be obeying PATH, not these rules

My instinct was that PATH is a Unix-ism and that the "real" search path that applications are supposed to use is determined from various registry entries in ways that only SearchPath knows about. But this is just an instinct, so it can be very wrong.

@dra27: you have access to MSVCRT sources. Could you please check how they implement execvp()? What path is searched and how? Then we should consider having the same logic for Unix.create_process, and perhaps for caml_search_exe_in_path too.

@vicuna
Copy link
Author

vicuna commented Oct 30, 2017

Comment author: clouds56

Sorry for misunderstanding SafeProcessSearchMode or SetSearchPathMode, it would only affect executable in current dir nothing to do with PATH.

BTW, I don't think that's behavior of SearchPath or Unix.create_process changed, it is windows add its "invalid" bash.exe into SYSTEM_DIR that is C:\Windows\System32\bash.exe in windows 10.
see https://github.com/Microsoft/WSL

@github-actions
Copy link

This issue has been open one year with no activity. Consequently, it is being marked with the "stale" label. What this means is that the issue will be automatically closed in 30 days unless more comments are added or the "stale" label is removed. Comments that provide new information on the issue are especially welcome: is it still reproducible? did it appear in other contexts? how critical is it? etc.

@github-actions github-actions bot added the Stale label Dec 28, 2020
@dra27 dra27 reopened this Apr 1, 2021
@dra27
Copy link
Member

dra27 commented Apr 1, 2021

Dropping this was a mistake on my part. This is definitely still an issue, and it's getting worse as Microsoft add more and more BSD tools to system32 - you now get the same problem with tar.exe.

I certainly think we should fix this. To address @xavierleroy's question above (finally): execvp behaves as you'd expect from Unix, obviously with a little bit of Windows extra flavour¹. In particular, the UCRT never uses SearchPath and the places it calls CreateProcessW (there are only two) both ensure that the filename passed will not trigger SearchPath - in particular, the spawnv implementation prefixes .\ to filenames.


¹ The flavour is the various rules for trying file extensions, always trying the current directory first, and the handling double-quotes when splitting PATH, which allows for the escaping of semicolons in PATH names.

@dra27
Copy link
Member

dra27 commented Apr 1, 2021

For the runtime, caml_search_exe_in_path should rarely do anything - off the top of my head, I don't know how you'd trigger caml_executable_name failing in native mode (so caml_search_exe_in_path is never called) and for bytecode I think you similarly need to try quite hard to set-up ocamlrun suddenly having to perform a search (IIRC argv[0] is a full path)

@github-actions
Copy link

github-actions bot commented Apr 4, 2022

This issue has been open one year with no activity. Consequently, it is being marked with the "stale" label. What this means is that the issue will be automatically closed in 30 days unless more comments are added or the "stale" label is removed. Comments that provide new information on the issue are especially welcome: is it still reproducible? did it appear in other contexts? how critical is it? etc.

@github-actions github-actions bot added the Stale label Apr 4, 2022
@dra27 dra27 removed the Stale label Apr 4, 2022
@dra27
Copy link
Member

dra27 commented Apr 4, 2022

I don't have an update for this per se, but it remains an issue which I intend either to get to or oversee being "got to"!

@github-actions
Copy link

github-actions bot commented Apr 7, 2023

This issue has been open one year with no activity. Consequently, it is being marked with the "stale" label. What this means is that the issue will be automatically closed in 30 days unless more comments are added or the "stale" label is removed. Comments that provide new information on the issue are especially welcome: is it still reproducible? did it appear in other contexts? how critical is it? etc.

@github-actions github-actions bot added the Stale label Apr 7, 2023
@github-actions github-actions bot closed this as completed May 8, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

3 participants