[PATCH 0/3] MR3839: kernel32: Add structure size checks to CreateActCtx().
Signed-off-by: Dmitry Timoshkov <dmitry(a)baikal.ru> -- https://gitlab.winehq.org/wine/wine/-/merge_requests/3839
From: Dmitry Timoshkov <dmitry(a)baikal.ru> Signed-off-by: Dmitry Timoshkov <dmitry(a)baikal.ru> --- dlls/kernel32/tests/actctx.c | 107 ++++++++++++++++++++++++++++++++++- 1 file changed, 106 insertions(+), 1 deletion(-) diff --git a/dlls/kernel32/tests/actctx.c b/dlls/kernel32/tests/actctx.c index d468777ac2f..61553703589 100644 --- a/dlls/kernel32/tests/actctx.c +++ b/dlls/kernel32/tests/actctx.c @@ -2783,18 +2783,123 @@ static void test_CreateActCtx(void) { static const DWORD flags[] = {LOAD_LIBRARY_AS_DATAFILE, LOAD_LIBRARY_AS_IMAGE_RESOURCE, LOAD_LIBRARY_AS_IMAGE_RESOURCE | LOAD_LIBRARY_AS_DATAFILE}; - CHAR path[MAX_PATH], dir[MAX_PATH], dll[MAX_PATH]; + static const struct + { + DWORD size, flags, error; + } test[] = + { + { FIELD_OFFSET(ACTCTXW, lpSource), 0, ERROR_INVALID_PARAMETER }, + { FIELD_OFFSET(ACTCTXW, wProcessorArchitecture), 0, 0 }, + { FIELD_OFFSET(ACTCTXW, lpAssemblyDirectory), ACTCTX_FLAG_ASSEMBLY_DIRECTORY_VALID, ERROR_INVALID_PARAMETER }, + { FIELD_OFFSET(ACTCTXW, lpResourceName), ACTCTX_FLAG_RESOURCE_NAME_VALID, ERROR_INVALID_PARAMETER }, + { FIELD_OFFSET(ACTCTXW, hModule), ACTCTX_FLAG_RESOURCE_NAME_VALID | ACTCTX_FLAG_HMODULE_VALID, ERROR_INVALID_PARAMETER }, + }; + char path[MAX_PATH], dir[MAX_PATH], dll[MAX_PATH], source[MAX_PATH]; + WCHAR pathW[MAX_PATH], dirW[MAX_PATH], sourceW[MAX_PATH]; ACTCTXA actctx; + ACTCTXW ctxW; HANDLE handle; int i; GetTempPathA(ARRAY_SIZE(path), path); + strcpy(dir, path); strcat(path, "main_wndcls.manifest"); write_manifest("testdep1.manifest", manifest_wndcls1); write_manifest("testdep2.manifest", manifest_wndcls2); write_manifest("main_wndcls.manifest", manifest_wndcls_main); + GetModuleFileNameA(NULL, source, ARRAY_SIZE(source)); + GetModuleFileNameW(NULL, sourceW, ARRAY_SIZE(sourceW)); + + GetTempPathW(ARRAY_SIZE(pathW), pathW); + wcscpy(dirW, pathW); + wcscat(pathW, L"main_wndcls.manifest"); + + memset(&ctxW, 0, sizeof(ctxW)); + ctxW.cbSize = sizeof(ctxW); + ctxW.dwFlags = ACTCTX_FLAG_ASSEMBLY_DIRECTORY_VALID | ACTCTX_FLAG_RESOURCE_NAME_VALID | ACTCTX_FLAG_HMODULE_VALID; + ctxW.lpSource = pathW; + ctxW.lpAssemblyDirectory = dirW; + ctxW.lpResourceName = (LPWSTR)124; + ctxW.hModule = GetModuleHandleW(NULL); + handle = CreateActCtxW(&ctxW); + ok(handle != INVALID_HANDLE_VALUE, "CreateActCtx error %lu\n", GetLastError()); + ReleaseActCtx(handle); + + for (i = 0; i < ARRAY_SIZE(test); i++) + { + winetest_push_context("%i", i); + ctxW.cbSize = test[i].size; + ctxW.dwFlags = test[i].flags; + SetLastError(0xdeadbeef); + handle = CreateActCtxW(&ctxW); + if (!test[i].error) + { + todo_wine + ok(handle != INVALID_HANDLE_VALUE, "CreateActCtx error %lu\n", GetLastError()); + ReleaseActCtx(handle); + } + else + { + ok(handle == INVALID_HANDLE_VALUE, "CreateActCtx should fail\n"); + ok(test[i].error == GetLastError(), "expected %lu, got %lu\n", test[i].error, GetLastError()); + } + + ctxW.cbSize += sizeof(void *); + if ((ctxW.dwFlags & (ACTCTX_FLAG_RESOURCE_NAME_VALID | ACTCTX_FLAG_HMODULE_VALID)) == ACTCTX_FLAG_RESOURCE_NAME_VALID) + ctxW.lpSource = sourceW; /* source without hModule must point to valid PE */ + SetLastError(0xdeadbeef); + handle = CreateActCtxW(&ctxW); + todo_wine_if(i != 4) + ok(handle != INVALID_HANDLE_VALUE, "CreateActCtx error %lu\n", GetLastError()); + ReleaseActCtx(handle); + + winetest_pop_context(); + } + + memset(&actctx, 0, sizeof(actctx)); + actctx.cbSize = sizeof(actctx); + actctx.dwFlags = ACTCTX_FLAG_ASSEMBLY_DIRECTORY_VALID | ACTCTX_FLAG_RESOURCE_NAME_VALID | ACTCTX_FLAG_HMODULE_VALID; + actctx.lpSource = path; + actctx.lpAssemblyDirectory = dir; + actctx.lpResourceName = (LPSTR)124; + actctx.hModule = GetModuleHandleW(NULL); + handle = CreateActCtxA(&actctx); + ok(handle != INVALID_HANDLE_VALUE, "CreateActCtx error %lu\n", GetLastError()); + ReleaseActCtx(handle); + + for (i = 0; i < ARRAY_SIZE(test); i++) + { + winetest_push_context("%i", i); + actctx.cbSize = test[i].size; + actctx.dwFlags = test[i].flags; + SetLastError(0xdeadbeef); + handle = CreateActCtxA(&actctx); + if (!test[i].error) + { + todo_wine + ok(handle != INVALID_HANDLE_VALUE, "CreateActCtx error %lu\n", GetLastError()); + ReleaseActCtx(handle); + } + else + { + ok(handle == INVALID_HANDLE_VALUE, "CreateActCtx should fail\n"); + ok(test[i].error == GetLastError(), "expected %lu, got %lu\n", test[i].error, GetLastError()); + } + + actctx.cbSize += sizeof(void *); + if ((actctx.dwFlags & (ACTCTX_FLAG_RESOURCE_NAME_VALID | ACTCTX_FLAG_HMODULE_VALID)) == ACTCTX_FLAG_RESOURCE_NAME_VALID) + actctx.lpSource = source; /* source without hModule must point to valid PE */ + SetLastError(0xdeadbeef); + handle = CreateActCtxA(&actctx); + todo_wine_if(i != 4) + ok(handle != INVALID_HANDLE_VALUE, "CreateActCtx error %lu\n", GetLastError()); + ReleaseActCtx(handle); + + winetest_pop_context(); + } + memset(&actctx, 0, sizeof(ACTCTXA)); actctx.cbSize = sizeof(ACTCTXA); actctx.lpSource = path; -- GitLab https://gitlab.winehq.org/wine/wine/-/merge_requests/3839
From: Dmitry Timoshkov <dmitry(a)baikal.ru> Signed-off-by: Dmitry Timoshkov <dmitry(a)baikal.ru> --- dlls/kernel32/process.c | 38 +++++++++++++++++++++++++++++++++++- dlls/kernel32/tests/actctx.c | 2 -- 2 files changed, 37 insertions(+), 3 deletions(-) diff --git a/dlls/kernel32/process.c b/dlls/kernel32/process.c index e9e18925911..a61bb826401 100644 --- a/dlls/kernel32/process.c +++ b/dlls/kernel32/process.c @@ -422,7 +422,7 @@ HANDLE WINAPI DECLSPEC_HOTPATCH CreateActCtxA( const ACTCTXA *actctx ) TRACE("%p %08lx\n", actctx, actctx ? actctx->dwFlags : 0); - if (!actctx || actctx->cbSize != sizeof(*actctx)) + if (!actctx || actctx->cbSize < FIELD_OFFSET(ACTCTXA, wProcessorArchitecture)) { SetLastError(ERROR_INVALID_PARAMETER); return INVALID_HANDLE_VALUE; @@ -440,11 +440,30 @@ HANDLE WINAPI DECLSPEC_HOTPATCH CreateActCtxA( const ACTCTXA *actctx ) actw.lpSource = src; if (actw.dwFlags & ACTCTX_FLAG_PROCESSOR_ARCHITECTURE_VALID) + { + if (actctx->cbSize < FIELD_OFFSET(ACTCTXA, wProcessorArchitecture) + sizeof(actctx->wProcessorArchitecture)) + { + SetLastError(ERROR_INVALID_PARAMETER); + goto done; + } actw.wProcessorArchitecture = actctx->wProcessorArchitecture; + } if (actw.dwFlags & ACTCTX_FLAG_LANGID_VALID) + { + if (actctx->cbSize < FIELD_OFFSET(ACTCTXA, wLangId) + sizeof(actctx->wLangId)) + { + SetLastError(ERROR_INVALID_PARAMETER); + goto done; + } actw.wLangId = actctx->wLangId; + } if (actw.dwFlags & ACTCTX_FLAG_ASSEMBLY_DIRECTORY_VALID) { + if (actctx->cbSize < FIELD_OFFSET(ACTCTXA, lpAssemblyDirectory) + sizeof(actctx->lpAssemblyDirectory)) + { + SetLastError(ERROR_INVALID_PARAMETER); + goto done; + } len = MultiByteToWideChar(CP_ACP, 0, actctx->lpAssemblyDirectory, -1, NULL, 0); assdir = HeapAlloc(GetProcessHeap(), 0, len * sizeof(WCHAR)); if (!assdir) goto done; @@ -453,6 +472,11 @@ HANDLE WINAPI DECLSPEC_HOTPATCH CreateActCtxA( const ACTCTXA *actctx ) } if (actw.dwFlags & ACTCTX_FLAG_RESOURCE_NAME_VALID) { + if (actctx->cbSize < FIELD_OFFSET(ACTCTXA, lpResourceName) + sizeof(actctx->lpResourceName)) + { + SetLastError(ERROR_INVALID_PARAMETER); + goto done; + } if ((ULONG_PTR)actctx->lpResourceName >> 16) { len = MultiByteToWideChar(CP_ACP, 0, actctx->lpResourceName, -1, NULL, 0); @@ -465,6 +489,11 @@ HANDLE WINAPI DECLSPEC_HOTPATCH CreateActCtxA( const ACTCTXA *actctx ) } if (actw.dwFlags & ACTCTX_FLAG_APPLICATION_NAME_VALID) { + if (actctx->cbSize < FIELD_OFFSET(ACTCTXA, lpApplicationName) + sizeof(actctx->lpApplicationName)) + { + SetLastError(ERROR_INVALID_PARAMETER); + goto done; + } len = MultiByteToWideChar(CP_ACP, 0, actctx->lpApplicationName, -1, NULL, 0); appname = HeapAlloc(GetProcessHeap(), 0, len * sizeof(WCHAR)); if (!appname) goto done; @@ -472,7 +501,14 @@ HANDLE WINAPI DECLSPEC_HOTPATCH CreateActCtxA( const ACTCTXA *actctx ) actw.lpApplicationName = appname; } if (actw.dwFlags & ACTCTX_FLAG_HMODULE_VALID) + { + if (actctx->cbSize < FIELD_OFFSET(ACTCTXA, hModule) + sizeof(actctx->hModule)) + { + SetLastError(ERROR_INVALID_PARAMETER); + goto done; + } actw.hModule = actctx->hModule; + } ret = CreateActCtxW(&actw); diff --git a/dlls/kernel32/tests/actctx.c b/dlls/kernel32/tests/actctx.c index 61553703589..98ca6c3cd76 100644 --- a/dlls/kernel32/tests/actctx.c +++ b/dlls/kernel32/tests/actctx.c @@ -2878,7 +2878,6 @@ static void test_CreateActCtx(void) handle = CreateActCtxA(&actctx); if (!test[i].error) { - todo_wine ok(handle != INVALID_HANDLE_VALUE, "CreateActCtx error %lu\n", GetLastError()); ReleaseActCtx(handle); } @@ -2893,7 +2892,6 @@ static void test_CreateActCtx(void) actctx.lpSource = source; /* source without hModule must point to valid PE */ SetLastError(0xdeadbeef); handle = CreateActCtxA(&actctx); - todo_wine_if(i != 4) ok(handle != INVALID_HANDLE_VALUE, "CreateActCtx error %lu\n", GetLastError()); ReleaseActCtx(handle); -- GitLab https://gitlab.winehq.org/wine/wine/-/merge_requests/3839
From: Dmitry Timoshkov <dmitry(a)baikal.ru> Signed-off-by: Dmitry Timoshkov <dmitry(a)baikal.ru> --- dlls/kernel32/tests/actctx.c | 2 -- dlls/ntdll/actctx.c | 56 ++++++++++++++++++++++++++++++++---- 2 files changed, 50 insertions(+), 8 deletions(-) diff --git a/dlls/kernel32/tests/actctx.c b/dlls/kernel32/tests/actctx.c index 98ca6c3cd76..585f72b9a75 100644 --- a/dlls/kernel32/tests/actctx.c +++ b/dlls/kernel32/tests/actctx.c @@ -2836,7 +2836,6 @@ static void test_CreateActCtx(void) handle = CreateActCtxW(&ctxW); if (!test[i].error) { - todo_wine ok(handle != INVALID_HANDLE_VALUE, "CreateActCtx error %lu\n", GetLastError()); ReleaseActCtx(handle); } @@ -2851,7 +2850,6 @@ static void test_CreateActCtx(void) ctxW.lpSource = sourceW; /* source without hModule must point to valid PE */ SetLastError(0xdeadbeef); handle = CreateActCtxW(&ctxW); - todo_wine_if(i != 4) ok(handle != INVALID_HANDLE_VALUE, "CreateActCtx error %lu\n", GetLastError()); ReleaseActCtx(handle); diff --git a/dlls/ntdll/actctx.c b/dlls/ntdll/actctx.c index 9f2c1d54720..8a9d8c9ae23 100644 --- a/dlls/ntdll/actctx.c +++ b/dlls/ntdll/actctx.c @@ -5251,12 +5251,15 @@ NTSTATUS WINAPI RtlCreateActivationContext( HANDLE *handle, const void *ptr ) TRACE("%p %08lx\n", pActCtx, pActCtx ? pActCtx->dwFlags : 0); - if (!pActCtx || pActCtx->cbSize < sizeof(*pActCtx) || + if (!pActCtx || pActCtx->cbSize < FIELD_OFFSET(ACTCTXW, wProcessorArchitecture) || (pActCtx->dwFlags & ~ACTCTX_FLAGS_ALL)) return STATUS_INVALID_PARAMETER; - if ((pActCtx->dwFlags & ACTCTX_FLAG_RESOURCE_NAME_VALID) && !pActCtx->lpResourceName) - return STATUS_INVALID_PARAMETER; + if (pActCtx->dwFlags & ACTCTX_FLAG_RESOURCE_NAME_VALID) + { + if (pActCtx->cbSize < FIELD_OFFSET(ACTCTXW, lpResourceName) + sizeof(pActCtx->lpResourceName) || !pActCtx->lpResourceName) + return STATUS_INVALID_PARAMETER; + } if (!(actctx = RtlAllocateHeap( GetProcessHeap(), HEAP_ZERO_MEMORY, sizeof(*actctx) ))) return STATUS_NO_MEMORY; @@ -5268,6 +5271,11 @@ NTSTATUS WINAPI RtlCreateActivationContext( HANDLE *handle, const void *ptr ) actctx->appdir.type = ACTIVATION_CONTEXT_PATH_TYPE_WIN32_FILE; if (pActCtx->dwFlags & ACTCTX_FLAG_APPLICATION_NAME_VALID) { + if (pActCtx->cbSize < FIELD_OFFSET(ACTCTXW, lpApplicationName) + sizeof(pActCtx->lpApplicationName)) + { + status = STATUS_INVALID_PARAMETER; + goto error; + } if (!(actctx->appdir.info = strdupW( pActCtx->lpApplicationName ))) goto error; } else @@ -5276,7 +5284,15 @@ NTSTATUS WINAPI RtlCreateActivationContext( HANDLE *handle, const void *ptr ) WCHAR *p; HMODULE module; - if (pActCtx->dwFlags & ACTCTX_FLAG_HMODULE_VALID) module = pActCtx->hModule; + if (pActCtx->dwFlags & ACTCTX_FLAG_HMODULE_VALID) + { + if (pActCtx->cbSize < FIELD_OFFSET(ACTCTXW, hModule) + sizeof(pActCtx->hModule)) + { + status = STATUS_INVALID_PARAMETER; + goto error; + } + module = pActCtx->hModule; + } else module = NtCurrentTeb()->Peb->ImageBaseAddress; if ((status = get_module_filename( module, &dir, 0 ))) goto error; @@ -5298,6 +5314,12 @@ NTSTATUS WINAPI RtlCreateActivationContext( HANDLE *handle, const void *ptr ) { DWORD dir_len, source_len; + if (pActCtx->cbSize < FIELD_OFFSET(ACTCTXW, lpAssemblyDirectory) + sizeof(pActCtx->lpAssemblyDirectory)) + { + status = STATUS_INVALID_PARAMETER; + goto error; + } + dir_len = wcslen(pActCtx->lpAssemblyDirectory); source_len = wcslen(pActCtx->lpSource); if (!(source = RtlAllocateHeap( GetProcessHeap(), 0, (dir_len+source_len+2)*sizeof(WCHAR)))) @@ -5331,11 +5353,33 @@ NTSTATUS WINAPI RtlCreateActivationContext( HANDLE *handle, const void *ptr ) acl.num_dependencies = 0; acl.allocated_dependencies = 0; - if (pActCtx->dwFlags & ACTCTX_FLAG_LANGID_VALID) lang = pActCtx->wLangId; - if (pActCtx->dwFlags & ACTCTX_FLAG_ASSEMBLY_DIRECTORY_VALID) directory = pActCtx->lpAssemblyDirectory; + if (pActCtx->dwFlags & ACTCTX_FLAG_LANGID_VALID) + { + if (pActCtx->cbSize < FIELD_OFFSET(ACTCTXW, wLangId) + sizeof(pActCtx->wLangId)) + { + status = STATUS_INVALID_PARAMETER; + goto error; + } + lang = pActCtx->wLangId; + } + if (pActCtx->dwFlags & ACTCTX_FLAG_ASSEMBLY_DIRECTORY_VALID) + { + if (pActCtx->cbSize < FIELD_OFFSET(ACTCTXW, lpAssemblyDirectory) + sizeof(pActCtx->lpAssemblyDirectory)) + { + status = STATUS_INVALID_PARAMETER; + goto error; + } + directory = pActCtx->lpAssemblyDirectory; + } if (pActCtx->dwFlags & ACTCTX_FLAG_RESOURCE_NAME_VALID) { + if (pActCtx->cbSize < FIELD_OFFSET(ACTCTXW, lpResourceName) + sizeof(pActCtx->lpResourceName)) + { + status = STATUS_INVALID_PARAMETER; + goto error; + } + /* if we have a resource it's a PE file */ if (pActCtx->dwFlags & ACTCTX_FLAG_HMODULE_VALID) { -- GitLab https://gitlab.winehq.org/wine/wine/-/merge_requests/3839
That's a lot of similar checks. Maybe estimate minimal size first and then check once? Also, ntdll function is not tested by itself, and its prototype was not validated as far as I can tell, so it's not possible to tell now where checks should be placed. Maybe having them only in kernelbase function is enough. -- https://gitlab.winehq.org/wine/wine/-/merge_requests/3839#note_45352
That's a lot of similar checks. Maybe estimate minimal size first and then check once? Also, ntdll function is not tested by itself, and its prototype was not validated as far as I can tell, so it's not possible to tell now where checks should be placed. Maybe having them only in kernelbase function is enough.
The checks are performed in a simiar way in other places. If you have a better idea how to do the size checks please share it. In any case CreateActCtxA() would have to perform the size check when converting the structure from A to W, and RtlCreateActivationContext() still needs to perform the check before accessing fields of the structure, so it seems unavoidable to have the checks in both places. There's no a single test in dlls/ntdll/tests for RtlCreateActivationContext(), so I'd guess that it's assumed that its functionality is covered by CreateActCtxW(), otherwise it's necessary to port all the exicting CreateActCtxW() tests for RtlCreateActivationContext(), do you really insist on it? -- https://gitlab.winehq.org/wine/wine/-/merge_requests/3839#note_45358
On Thu Sep 14 16:12:06 2023 +0000, Dmitry Timoshkov wrote:
That's a lot of similar checks. Maybe estimate minimal size first and then check once? Also, ntdll function is not tested by itself, and its prototype was not validated as far as I can tell, so it's not possible to tell now where checks should be placed. Maybe having them only in kernelbase function is enough. The checks are performed in a simiar way in other places. If you have a better idea how to do the size checks please share it. In any case CreateActCtxA() would have to perform the size check when converting the structure from A to W, and RtlCreateActivationContext() still needs to perform the check before accessing fields of the structure, so it seems unavoidable to have the checks in both places. There's no a single test in dlls/ntdll/tests for RtlCreateActivationContext(), so I'd guess that it's assumed that its functionality is covered by CreateActCtxW(), otherwise it's necessary to port all the exicting CreateActCtxW() tests for RtlCreateActivationContext(), do you really insist on it? I don't insist on anything, but only pointing at potential improvements.
-- https://gitlab.winehq.org/wine/wine/-/merge_requests/3839#note_45516
I don't insist on anything, but only pointing at potential improvements.
I don't see how to avoid the checks in both A and W versions. If you have an idea how to do that please share it. -- https://gitlab.winehq.org/wine/wine/-/merge_requests/3839#note_45517
On Fri Sep 15 11:30:54 2023 +0000, Dmitry Timoshkov wrote:
I don't insist on anything, but only pointing at potential improvements. I don't see how to avoid the checks in both A and W versions. If you have an idea how to do that please share it. I was thinking something like that:
``` if (flags & FLAG_1) size = max(size, FIELDOFFSET(..) + ...); if (flags & FLAG_2) size = max(size, ...); if (cbSize < size) { SetLastError(...); return; } ``` To make it easier to read you can use just FIELDOFFSET() of the next field. I think it's fine to duplicate that for A and W functions, and avoid changing Rtl* one at all, as long as we don't provide compatible interface for it, nothing is going to call it successfully anyway. -- https://gitlab.winehq.org/wine/wine/-/merge_requests/3839#note_45542
I was thinking something like that:
``` if (flags & FLAG_1) size = max(size, FIELDOFFSET(..) + ...); if (flags & FLAG_2) size = max(size, ...);
if (cbSize < size) { SetLastError(...); return; } ```
To make it easier to read you can use just FIELDOFFSET() of the next field.
Thanks for the suggestion. It seems that it won't significantly improve readability or simplify the code, so I'd prefer to stick with my solution.
I think it's fine to duplicate that for A and W functions, and avoid changing Rtl* one at all, as long as we don't provide compatible interface for it, nothing is going to call it successfully anyway.
RtlCreateActivationContext() already performs the checks for the flags and structure field contents, so discussing where the checks belong is out of the scope of this MR. If you have justified reasons to doubt about current implementation feel free to add appropriate tests. -- https://gitlab.winehq.org/wine/wine/-/merge_requests/3839#note_45543
On Fri Sep 15 17:04:25 2023 +0000, Dmitry Timoshkov wrote:
I was thinking something like that:
``` if (flags & FLAG_1) size = max(size, FIELDOFFSET(..) + ...); if (flags & FLAG_2) size = max(size, ...);
if (cbSize < size) { SetLastError(...); return; } ```
To make it easier to read you can use just FIELDOFFSET() of the next field. Thanks for the suggestion. It seems that it won't significantly improve readability or simplify the code, so I'd prefer to stick with my solution. I think it's fine to duplicate that for A and W functions, and avoid changing Rtl* one at all, as long as we don't provide compatible interface for it, nothing is going to call it successfully anyway. RtlCreateActivationContext() already performs the checks for the flags and structure field contents, so discussing where the checks belong is out of the scope of this MR. If you have justified reasons to doubt about current implementation feel free to add appropriate tests. I was thinking about doing it like this:
``` 443 size = FIELD_OFFSET(ACTCTXA, wProcessorArchitecture); 444 if (actctx->dwFlags & ACTCTX_FLAG_PROCESSOR_ARCHITECTURE_VALID) 445 size = max(size, FIELD_OFFSET(ACTCTXA, wLangId)); 446 if (actctx->dwFlags & ACTCTX_FLAG_LANGID_VALID) 447 size = max(size, FIELD_OFFSET(ACTCTXA, lpAssemblyDirectory)); 448 if (actctx->dwFlags & ACTCTX_FLAG_ASSEMBLY_DIRECTORY_VALID) 449 size = max(size, FIELD_OFFSET(ACTCTXA, lpResourceName)); 450 if (actctx->dwFlags & ACTCTX_FLAG_RESOURCE_NAME_VALID) 451 size = max(size, FIELD_OFFSET(ACTCTXA, lpApplicationName)); 452 if (actctx->dwFlags & ACTCTX_FLAG_APPLICATION_NAME_VALID) 453 size = max(size, FIELD_OFFSET(ACTCTXA, hModule)); 454 if (actctx->dwFlags & ACTCTX_FLAG_HMODULE_VALID) 455 size = sizeof(ACTCTXA); 456 if (actctx->cbSize < size) 457 { 458 SetLastError(ERROR_INVALID_PARAMETER); 459 return INVALID_HANDLE_VALUE; 460 } ``` Then duplicating exactly the same check in CreateActCtxW(), structure layout is the same. This way we won't need six size checks and six jumps, times two. Thanks for the bug report (https://bugs.winehq.org/show_bug.cgi?id=55782). -- https://gitlab.winehq.org/wine/wine/-/merge_requests/3839#note_48679
On Fri Oct 13 20:21:09 2023 +0000, Nikolay Sivov wrote:
I was thinking about doing it like this: ``` 443 size = FIELD_OFFSET(ACTCTXA, wProcessorArchitecture); 444 if (actctx->dwFlags & ACTCTX_FLAG_PROCESSOR_ARCHITECTURE_VALID) 445 size = max(size, FIELD_OFFSET(ACTCTXA, wLangId)); 446 if (actctx->dwFlags & ACTCTX_FLAG_LANGID_VALID) 447 size = max(size, FIELD_OFFSET(ACTCTXA, lpAssemblyDirectory)); 448 if (actctx->dwFlags & ACTCTX_FLAG_ASSEMBLY_DIRECTORY_VALID) 449 size = max(size, FIELD_OFFSET(ACTCTXA, lpResourceName)); 450 if (actctx->dwFlags & ACTCTX_FLAG_RESOURCE_NAME_VALID) 451 size = max(size, FIELD_OFFSET(ACTCTXA, lpApplicationName)); 452 if (actctx->dwFlags & ACTCTX_FLAG_APPLICATION_NAME_VALID) 453 size = max(size, FIELD_OFFSET(ACTCTXA, hModule)); 454 if (actctx->dwFlags & ACTCTX_FLAG_HMODULE_VALID) 455 size = sizeof(ACTCTXA); 456 if (actctx->cbSize < size) 457 { 458 SetLastError(ERROR_INVALID_PARAMETER); 459 return INVALID_HANDLE_VALUE; 460 } ``` Then duplicating exactly the same check in CreateActCtxW(), structure layout is the same. This way we won't need six size checks and six jumps, times two. Thanks for the bug report (https://bugs.winehq.org/show_bug.cgi?id=55782). You could even use an array of flags and their corresponding sizes.
-- https://gitlab.winehq.org/wine/wine/-/merge_requests/3839#note_48680
I was thinking about doing it like this:
``` 443 size = FIELD_OFFSET(ACTCTXA, wProcessorArchitecture); 444 if (actctx->dwFlags & ACTCTX_FLAG_PROCESSOR_ARCHITECTURE_VALID) 445 size = max(size, FIELD_OFFSET(ACTCTXA, wLangId)); 446 if (actctx->dwFlags & ACTCTX_FLAG_LANGID_VALID) 447 size = max(size, FIELD_OFFSET(ACTCTXA, lpAssemblyDirectory)); 448 if (actctx->dwFlags & ACTCTX_FLAG_ASSEMBLY_DIRECTORY_VALID) 449 size = max(size, FIELD_OFFSET(ACTCTXA, lpResourceName)); 450 if (actctx->dwFlags & ACTCTX_FLAG_RESOURCE_NAME_VALID) 451 size = max(size, FIELD_OFFSET(ACTCTXA, lpApplicationName)); 452 if (actctx->dwFlags & ACTCTX_FLAG_APPLICATION_NAME_VALID) 453 size = max(size, FIELD_OFFSET(ACTCTXA, hModule)); 454 if (actctx->dwFlags & ACTCTX_FLAG_HMODULE_VALID) 455 size = sizeof(ACTCTXA); 456 if (actctx->cbSize < size) 457 { 458 SetLastError(ERROR_INVALID_PARAMETER); 459 return INVALID_HANDLE_VALUE; 460 } ```
Then duplicating exactly the same check in CreateActCtxW(), structure layout is the same. This way we won't need six size checks and six jumps, times two.
Yes, this avoids duplicate size checks, however it introduces duplicate dwFlags checks and makes things even worse with an influx of max() invocations that lead to actually duplicating of the size checks. I don't like what I see in the generated assebler. Why do you think that's a better approach? -- https://gitlab.winehq.org/wine/wine/-/merge_requests/3839#note_48682
participants (4)
-
Alexandre Julliard (＠julliard) -
Dmitry Timoshkov
-
Dmitry Timoshkov (＠dmitry)
-
Nikolay Sivov (＠nsivov)