| Message ID | 20220613162626.11541-2-nil-admirari@mailo.com |
|---|---|
| State | New |
| Headers | show |
| Series | [FFmpeg-devel,v14,1/5] libavutil: Add wchartoutf8(), wchartoansi(), utf8toansi() and getenv_utf8() | expand |
| Context | Check | Description |
|---|---|---|
| andriy/make_x86 | success | Make finished |
| andriy/make_fate_x86 | success | Make fate finished |
> -----Original Message----- > From: ffmpeg-devel <ffmpeg-devel-bounces@ffmpeg.org> On Behalf Of Nil > Admirari > Sent: Monday, June 13, 2022 6:26 PM > To: ffmpeg-devel@ffmpeg.org > Subject: [FFmpeg-devel] [PATCH v14 2/5] compat/w32dlfcn.h: Remove > MAX_PATH limit and replace LoadLibraryExA with LoadLibraryExW > > --- > compat/w32dlfcn.h | 80 ++++++++++++++++++++++++++++++++++------- > -- > libavcodec/mf_utils.h | 1 + > 2 files changed, 65 insertions(+), 16 deletions(-) > > diff --git a/compat/w32dlfcn.h b/compat/w32dlfcn.h > index 52a94efafb..e49d3841aa 100644 > --- a/compat/w32dlfcn.h > +++ b/compat/w32dlfcn.h > @@ -22,9 +22,31 @@ > #ifdef _WIN32 > #include <windows.h> > #include "config.h" > -#if (_WIN32_WINNT < 0x0602) || HAVE_WINRT > #include "libavutil/wchar_filename.h" > -#endif > + > +static inline wchar_t *get_module_filename(HMODULE module) > +{ > + wchar_t *path = NULL, *new_path; > + DWORD path_size = 0, path_len; > + > + do { > + path_size = path_size ? 2 * path_size : MAX_PATH; > + new_path = av_realloc_array(path, path_size, sizeof *path); > + if (!new_path) { > + av_free(path); > + return NULL; > + } > + path = new_path; > + path_len = GetModuleFileNameW(module, path, path_size); > + } while (path_len && path_size <= 32768 && path_size <= > path_len); Why do you use a 'do' loop? Can't you use the normal 2-step approach, i.e. call the winapi function with a NULL buffer, and then use the returned size to allocate the buffer. This way you always need a single allocation only. softworkz
> Why do you use a 'do' loop? Can't you use the normal 2-step > approach, i.e. call the winapi function with a NULL buffer, > and then use the returned size to allocate the buffer. > This way you always need a single allocation only. GetModuleFileNameW does not follow this convention: https://docs.microsoft.com/en-us/windows/win32/api/libloaderapi/nf-libloaderapi-getmodulefilenamew > If the length of the path exceeds the size that the nSize parameter specifies, > the function succeeds and the string is truncated to nSize characters > including the terminating null character. MS does the looping too in their WIL library: https://github.com/microsoft/wil/blob/master/include/wil/win32_helpers.h#L339-L341.
> -----Original Message----- > From: ffmpeg-devel <ffmpeg-devel-bounces@ffmpeg.org> On Behalf Of > nil-admirari@mailo.com > Sent: Monday, June 13, 2022 7:03 PM > To: ffmpeg-devel@ffmpeg.org > Subject: Re: [FFmpeg-devel] [PATCH v14 2/5] compat/w32dlfcn.h: Remove > MAX_PATH limit and replace LoadLibraryExA with LoadLibraryExW > > > Why do you use a 'do' loop? Can't you use the normal 2-step > > approach, i.e. call the winapi function with a NULL buffer, > > and then use the returned size to allocate the buffer. > > This way you always need a single allocation only. > > GetModuleFileNameW does not follow this convention: > https://docs.microsoft.com/en-us/windows/win32/api/libloaderapi/nf- > libloaderapi-getmodulefilenamew > > > If the length of the path exceeds the size that the nSize parameter > specifies, > > the function succeeds and the string is truncated to nSize > characters > > including the terminating null character. > > MS does the looping too in their WIL library: > https://github.com/microsoft/wil/blob/master/include/wil/win32_helper > s.h#L339-L341. Yes, you're right in this case; hadn't looked it up. Thanks for the pointers. sw
> -----Original Message----- > From: ffmpeg-devel <ffmpeg-devel-bounces@ffmpeg.org> On Behalf Of Nil > Admirari > Sent: Monday, June 13, 2022 6:26 PM > To: ffmpeg-devel@ffmpeg.org > Subject: [FFmpeg-devel] [PATCH v14 2/5] compat/w32dlfcn.h: Remove > MAX_PATH limit and replace LoadLibraryExA with LoadLibraryExW > > --- > compat/w32dlfcn.h | 80 ++++++++++++++++++++++++++++++++++------- > -- > libavcodec/mf_utils.h | 1 + > 2 files changed, 65 insertions(+), 16 deletions(-) > > diff --git a/compat/w32dlfcn.h b/compat/w32dlfcn.h > index 52a94efafb..e49d3841aa 100644 > --- a/compat/w32dlfcn.h > +++ b/compat/w32dlfcn.h > @@ -22,9 +22,31 @@ > #ifdef _WIN32 > #include <windows.h> > #include "config.h" > -#if (_WIN32_WINNT < 0x0602) || HAVE_WINRT > #include "libavutil/wchar_filename.h" > -#endif > + > +static inline wchar_t *get_module_filename(HMODULE module) > +{ > + wchar_t *path = NULL, *new_path; > + DWORD path_size = 0, path_len; > + > + do { > + path_size = path_size ? 2 * path_size : MAX_PATH; > + new_path = av_realloc_array(path, path_size, sizeof *path); > + if (!new_path) { > + av_free(path); > + return NULL; > + } > + path = new_path; > + path_len = GetModuleFileNameW(module, path, path_size); > + } while (path_len && path_size <= 32768 && path_size <= > path_len); path_size <= INT16_MAX (the edge case is already covered by the equals) > + > + if (!path_len) { > + av_free(path); > + return NULL; > + } > + return path; > +} > + > /** > * Safe function used to open dynamic libs. This attempts to improve > program security > * by removing the current directory from the dll search path. Only > dll's found in the > @@ -34,29 +56,53 @@ > */ > static inline HMODULE win32_dlopen(const char *name) > { > + wchar_t *name_w = NULL; > + if (utf8towchar(name, &name_w)) > + name_w = NULL; > #if _WIN32_WINNT < 0x0602 > // Need to check if KB2533623 is available I know this line existed before your path, but it would be nice to clarify check and condition, like: // On Win7 an earlier we check if KB2533623 is available > if (!GetProcAddress(GetModuleHandleW(L"kernel32.dll"), > "SetDefaultDllDirectories")) { > HMODULE module = NULL; > - wchar_t *path = NULL, *name_w = NULL; > - DWORD pathlen; > - if (utf8towchar(name, &name_w)) > + wchar_t *path = NULL, *new_path; > + DWORD pathlen, pathsize, namelen; > + if (!name_w) > goto exit; > - path = (wchar_t *)av_calloc(MAX_PATH, sizeof(wchar_t)); > + namelen = wcslen(name_w); > // Try local directory first > - pathlen = GetModuleFileNameW(NULL, path, MAX_PATH); > - pathlen = wcsrchr(path, '\\') - path; > - if (pathlen == 0 || pathlen + wcslen(name_w) + 2 > MAX_PATH) > + path = get_module_filename(NULL); > + if (!path) > goto exit; > - path[pathlen] = '\\'; > + new_path = wcsrchr(path, '\\'); > + if (!new_path) > + goto exit; > + pathlen = new_path - path; > + pathsize = pathlen + namelen + 2; > + new_path = av_realloc_array(path, pathsize, sizeof *path); > + if (!new_path) > + goto exit; > + path = new_path; > wcscpy(path + pathlen + 1, name_w); > module = LoadLibraryExW(path, NULL, > LOAD_WITH_ALTERED_SEARCH_PATH); > if (module == NULL) { > // Next try System32 directory > - pathlen = GetSystemDirectoryW(path, MAX_PATH); > - if (pathlen == 0 || pathlen + wcslen(name_w) + 2 > > MAX_PATH) > + pathlen = GetSystemDirectoryW(path, pathsize); > + if (!pathlen) > goto exit; > - path[pathlen] = '\\'; > + // Buffer is not enough in two cases: > + // 1. system directory + \ + module name > + // 2. system directory even without the module name. > + if (pathlen + namelen + 2 > pathsize) { > + pathsize = pathlen + namelen + 2; > + new_path = av_realloc_array(path, pathsize, sizeof > *path); > + if (!new_path) > + goto exit; > + path = new_path; > + // Query again to handle the case #2. > + pathlen = GetSystemDirectoryW(path, pathsize); > + if (!pathlen) > + goto exit; > + } > + path[pathlen] = L'\\'; > wcscpy(path + pathlen + 1, name_w); > module = LoadLibraryExW(path, NULL, > LOAD_WITH_ALTERED_SEARCH_PATH); > } > @@ -73,15 +119,17 @@ exit: > # define LOAD_LIBRARY_SEARCH_SYSTEM32 0x00000800 > #endif > #if HAVE_WINRT > - wchar_t *name_w = NULL; > int ret; > - if (utf8towchar(name, &name_w)) > + if (!name_w) > return NULL; > ret = LoadPackagedLibrary(name_w, 0); > av_free(name_w); > return ret; > #else > - return LoadLibraryExA(name, NULL, > LOAD_LIBRARY_SEARCH_APPLICATION_DIR | LOAD_LIBRARY_SEARCH_SYSTEM32); > + /* filename may be be in CP_ACP */ > + if (!name_w) > + return LoadLibraryExA(name, NULL, > LOAD_LIBRARY_SEARCH_APPLICATION_DIR | LOAD_LIBRARY_SEARCH_SYSTEM32); > + return LoadLibraryExW(name_w, NULL, > LOAD_LIBRARY_SEARCH_APPLICATION_DIR | LOAD_LIBRARY_SEARCH_SYSTEM32); name_w leaks here
> path_size <= INT16_MAX > > (the edge case is already covered by the equals) Done: https://ffmpeg.org/pipermail/ffmpeg-devel/2022-June/297590.html. Don't quite understand what edge case you've meant: INT16_MAX is 32767, which is the maximal path length allowed, + 1 is needed for the terminating null. > I know this line existed before your path, but it would be nice > to clarify check and condition, like: > > // On Win7 an earlier we check if KB2533623 is available Changed the comment. > name_w leaks here Fixed.
> -----Original Message----- > From: ffmpeg-devel <ffmpeg-devel-bounces@ffmpeg.org> On Behalf Of > nil-admirari@mailo.com > Sent: Wednesday, June 15, 2022 10:00 PM > To: ffmpeg-devel@ffmpeg.org > Subject: Re: [FFmpeg-devel] [PATCH v14 2/5] compat/w32dlfcn.h: Remove > MAX_PATH limit and replace LoadLibraryExA with LoadLibraryExW > > > path_size <= INT16_MAX > > > > (the edge case is already covered by the equals) > > Done: https://ffmpeg.org/pipermail/ffmpeg-devel/2022- > June/297590.html. > Don't quite understand what edge case you've meant: > INT16_MAX is 32767, which is the maximal path length allowed, > + 1 is needed for the terminating null. With the +1 your while condition term is effectively path_size <= 32768 But when the path_size is 32768, you do not need to go for another loop with an increased buffer because this is already as large as it can get. There won't be any 32769 or 32770 (...) cases, I think. > > I know this line existed before your path, but it would be nice > > to clarify check and condition, like: > > > > // On Win7 an earlier we check if KB2533623 is available > > Changed the comment. Cool. Thanks. softworkz > > > name_w leaks here > > Fixed.
> With the +1 your while condition term is effectively > > path_size <= 32768 > > But when the path_size is 32768, you do not need to > go for another loop with an increased buffer because this is > already as large as it can get. There won't be any 32769 > or 32770 (...) cases, I think. Removed +1: https://ffmpeg.org/pipermail/ffmpeg-devel/2022-June/297688.html.
diff --git a/compat/w32dlfcn.h b/compat/w32dlfcn.h index 52a94efafb..e49d3841aa 100644 --- a/compat/w32dlfcn.h +++ b/compat/w32dlfcn.h @@ -22,9 +22,31 @@ #ifdef _WIN32 #include <windows.h> #include "config.h" -#if (_WIN32_WINNT < 0x0602) || HAVE_WINRT #include "libavutil/wchar_filename.h" -#endif + +static inline wchar_t *get_module_filename(HMODULE module) +{ + wchar_t *path = NULL, *new_path; + DWORD path_size = 0, path_len; + + do { + path_size = path_size ? 2 * path_size : MAX_PATH; + new_path = av_realloc_array(path, path_size, sizeof *path); + if (!new_path) { + av_free(path); + return NULL; + } + path = new_path; + path_len = GetModuleFileNameW(module, path, path_size); + } while (path_len && path_size <= 32768 && path_size <= path_len); + + if (!path_len) { + av_free(path); + return NULL; + } + return path; +} + /** * Safe function used to open dynamic libs. This attempts to improve program security * by removing the current directory from the dll search path. Only dll's found in the @@ -34,29 +56,53 @@ */ static inline HMODULE win32_dlopen(const char *name) { + wchar_t *name_w = NULL; + if (utf8towchar(name, &name_w)) + name_w = NULL; #if _WIN32_WINNT < 0x0602 // Need to check if KB2533623 is available if (!GetProcAddress(GetModuleHandleW(L"kernel32.dll"), "SetDefaultDllDirectories")) { HMODULE module = NULL; - wchar_t *path = NULL, *name_w = NULL; - DWORD pathlen; - if (utf8towchar(name, &name_w)) + wchar_t *path = NULL, *new_path; + DWORD pathlen, pathsize, namelen; + if (!name_w) goto exit; - path = (wchar_t *)av_calloc(MAX_PATH, sizeof(wchar_t)); + namelen = wcslen(name_w); // Try local directory first - pathlen = GetModuleFileNameW(NULL, path, MAX_PATH); - pathlen = wcsrchr(path, '\\') - path; - if (pathlen == 0 || pathlen + wcslen(name_w) + 2 > MAX_PATH) + path = get_module_filename(NULL); + if (!path) goto exit; - path[pathlen] = '\\'; + new_path = wcsrchr(path, '\\'); + if (!new_path) + goto exit; + pathlen = new_path - path; + pathsize = pathlen + namelen + 2; + new_path = av_realloc_array(path, pathsize, sizeof *path); + if (!new_path) + goto exit; + path = new_path; wcscpy(path + pathlen + 1, name_w); module = LoadLibraryExW(path, NULL, LOAD_WITH_ALTERED_SEARCH_PATH); if (module == NULL) { // Next try System32 directory - pathlen = GetSystemDirectoryW(path, MAX_PATH); - if (pathlen == 0 || pathlen + wcslen(name_w) + 2 > MAX_PATH) + pathlen = GetSystemDirectoryW(path, pathsize); + if (!pathlen) goto exit; - path[pathlen] = '\\'; + // Buffer is not enough in two cases: + // 1. system directory + \ + module name + // 2. system directory even without the module name. + if (pathlen + namelen + 2 > pathsize) { + pathsize = pathlen + namelen + 2; + new_path = av_realloc_array(path, pathsize, sizeof *path); + if (!new_path) + goto exit; + path = new_path; + // Query again to handle the case #2. + pathlen = GetSystemDirectoryW(path, pathsize); + if (!pathlen) + goto exit; + } + path[pathlen] = L'\\'; wcscpy(path + pathlen + 1, name_w); module = LoadLibraryExW(path, NULL, LOAD_WITH_ALTERED_SEARCH_PATH); } @@ -73,15 +119,17 @@ exit: # define LOAD_LIBRARY_SEARCH_SYSTEM32 0x00000800 #endif #if HAVE_WINRT - wchar_t *name_w = NULL; int ret; - if (utf8towchar(name, &name_w)) + if (!name_w) return NULL; ret = LoadPackagedLibrary(name_w, 0); av_free(name_w); return ret; #else - return LoadLibraryExA(name, NULL, LOAD_LIBRARY_SEARCH_APPLICATION_DIR | LOAD_LIBRARY_SEARCH_SYSTEM32); + /* filename may be be in CP_ACP */ + if (!name_w) + return LoadLibraryExA(name, NULL, LOAD_LIBRARY_SEARCH_APPLICATION_DIR | LOAD_LIBRARY_SEARCH_SYSTEM32); + return LoadLibraryExW(name_w, NULL, LOAD_LIBRARY_SEARCH_APPLICATION_DIR | LOAD_LIBRARY_SEARCH_SYSTEM32); #endif } #define dlopen(name, flags) win32_dlopen(name) diff --git a/libavcodec/mf_utils.h b/libavcodec/mf_utils.h index 3b12344f3e..aebfb9ad21 100644 --- a/libavcodec/mf_utils.h +++ b/libavcodec/mf_utils.h @@ -29,6 +29,7 @@ // mf*.h headers below indirectly include strmif.h.) #include <icodecapi.h> #else +#define NO_DSHOW_STRSAFE #include <dshow.h> // Older versions of mingw-w64 need codecapi.h explicitly included, while newer // ones include it implicitly from dshow.h (via uuids.h).