Make win32_pathbyaddr more reliable#30705
Conversation
A user has reported that win32_pathbyaddr can be unreliable in multithreaded environments. See: https://learn.microsoft.com/en-us/windows/win32/api/tlhelp32/nf-tlhelp32-createtoolhelp32snapshot Specifically they have observed the following behavior, as noted in the above article: When taking snapshots that include heaps and modules for a process other than the current process, the CreateToolhelp32Snapshot function can fail or return incorrect information for a variety of reasons. For example, if the loader data table in the target process is corrupted or not initialized, or if the module list changes during the function call as a result of DLLs being loaded or unloaded, the function might fail with ERROR_BAD_LENGTH or other error code. Ensure that the target process was not started in a suspended state, and try calling the function again. If the function fails with ERROR_BAD_LENGTH when called with TH32CS_SNAPMODULE or TH32CS_SNAPMODULE32, call the function again until it succeeds. This behavior necessitates calling DSO_pathbyaddr mutiple times to get a succesful return code. win32_pathbyaddr can be made more reliable, avoiding the need for multiple calls by using alternate windows apis that are not/less succeptible to these transient errors in multithreaded environments. refactor win32_pathbyaddr here to implement that increased reliability.
bob-beck
left a comment
There was a problem hiding this comment.
Presuming that this will blow up immediately if it doesn't work in some older environments and we have test coverage for such things. probably OK?
but that's a bit of an "if?"
|
@bob-beck yeah, thats always a possibility, but I checked on all the api calls in question, and they are all supported back to windows xp, save for one (can't remember which), that was introduced in windows server 2000. So I think we're safe here. |
| return -1; | ||
| if (sz <= 0) { | ||
| OPENSSL_free(wpath); | ||
| return (int)(wlen + 1); |
There was a problem hiding this comment.
Here, we should also return the utf8len computed after this block. Otherwise, the return value will differ. So something like this:
utf8len = WideCharToMultiByte(CP_UTF8, 0, wpath, -1, NULL, 0, NULL, NULL);
if (sz <= 0) {
OPENSSL_free(wpath);
return utf8len;
}There was a problem hiding this comment.
I don't think what you're saying is right. You're correct in that returning wlen + 1 will require a buffer size greater than the size required to store the final converted utf8 string, but we have to do that, because we need a buffer large enough to hold the 2 byte widechar array returned from GetModuleFileNameW().
Sashan
left a comment
There was a problem hiding this comment.
looks good (apart from hModule handle leak.
also this PR kills code for WIN_CE. IMO it's OK (given code goes to master). I'm just saying it loudly here. So people are aware. It kill WIN_CE friendliness here will become a problem later then we can resurrect old code from history and use it under WIN_CE guard.
Sashan
left a comment
There was a problem hiding this comment.
looks good to me. thanks.
| }; | ||
| addr = t.p; | ||
| HMODULE hModule = NULL; | ||
| const DWORD wpathSize = 32768; /* 32768 is the maximum possible path length on Windows */ |
| GetLastError()); | ||
| goto out; | ||
| } | ||
| wpath = (WCHAR *)OPENSSL_malloc(wpathSize * sizeof(WCHAR)); |
There was a problem hiding this comment.
OPENSSL_malloc_array(wpathSize, sizeof(WCHAR))
A user has reported that win32_pathbyaddr can be unreliable in multithreaded environments. See:
https://learn.microsoft.com/en-us/windows/win32/api/tlhelp32/nf-tlhelp32-createtoolhelp32snapshot
Specifically they have observed the following behavior, as noted in the above article:
When taking snapshots that include heaps and modules for a process other than the current process, the CreateToolhelp32Snapshot function can fail or return incorrect information for a variety of reasons. For example, if the loader data table in the target process is corrupted or not initialized, or if the module list changes during the function call as a result of DLLs being loaded or unloaded, the function might fail with ERROR_BAD_LENGTH or other error code. Ensure that the target process was not started in a suspended state, and try calling the function again. If the function fails with ERROR_BAD_LENGTH when called with TH32CS_SNAPMODULE or TH32CS_SNAPMODULE32, call the function again until it succeeds.
This behavior necessitates calling DSO_pathbyaddr mutiple times to get a succesful return code.
win32_pathbyaddr can be made more reliable, avoiding the need for multiple calls by using alternate windows apis that are not/less succeptible to these transient errors in multithreaded environments.
refactor win32_pathbyaddr here to implement that increased reliability.