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 GetModuleFileName in GetProcessPath #110888

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

huoyaoyuan
Copy link
Member

Fixes #110800

@dotnet-policy-service dotnet-policy-service bot added the community-contribution Indicates that the PR has been added by a community member label Dec 22, 2024
@jkotas
Copy link
Member

jkotas commented Dec 22, 2024

Ideally, we would have a test for this. Unfortunately, it is not easy to build one because of other issues in the end-to-end handling of longs paths (#58492).

Have you done ad-hoc testing for this? Does the API return the path with or without \\?\ prefix?

@huoyaoyuan
Copy link
Member Author

Ad-hoc testing passed, path is returned without \\?\ prefix:

image

Explorer refuses to create folder or file exceeding MAX_PATH, and PowerShell can't start process exceeding MAX_PATH. cmd can launch the executable.

@huoyaoyuan
Copy link
Member Author

Does there need to be a bound to prevent integer overflow? It would cause infinite loop again if path length exceeds 2^30, which is unlikely to happen in any situation.

@jkotas
Copy link
Member

jkotas commented Dec 24, 2024

It would cause infinite loop again if path length exceeds 2^30, which is unlikely to happen in any situation.

I do not think it would lead to infinite loop. EnsureCapacity would throw an exception once there is an overflow and it gets called with negative capacity.

@jborean93
Copy link
Contributor

Ad-hoc testing passed, path is returned without \?\ prefix:

That's interesting, in my manual test in #110800 using PInvoke to call GetModulePath it returned the \\?\ prefix. I wonder if it's due to how the executable was started because I was setting the path to CreateProcess with that prefix.

@am11
Copy link
Member

am11 commented Dec 28, 2024

Seems like there is an opportunity for consolidation here, as logically the control wouldn't reach this point if the runtime/corehost initialization had this logic flawed; so corehost (and only corehost) is doing it right. Maybe it's better to delete calls from all these places:

  1. bool GetModuleFileNameWrapper(HMODULE hModule, pal::string_t* recv)
  2. inline string_t get_exe_path()
  3. REDHAWK_PALEXPORT UInt32_BOOL REDHAWK_PALAPI PalAllocateThunksFromTemplate(_In_ HANDLE hTemplateModule, uint32_t templateRva, size_t templateSize, _Outptr_result_bytebuffer_(templateSize) void** newThunksOut)

(there maybe others)

in favor of

if (GetModuleFileNameA(NULL, path, MAX_PATH) == 0)
and update it to the logic used in hostmisc/pal.windows.cpp. After that, use this minipal_getexepath() as we do on unix repo-wide (and windows+unix on mono).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area-System.Runtime community-contribution Indicates that the PR has been added by a community member
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Environment.ProcessPath hangs with exe path > MAX_PATH
4 participants