[PATCH 0/1] MR11014: setupapi: Initialize destW in SetupCopyOEMInfA (ASan).
Otherwise the call to WideCharToMultiByte does a lstrlenW, which may overrun the uninitialized destW buffer. This showed up in the [last ASan job](https://gitlab.winehq.org/bernhardu/wine/-/jobs/271183#L3584) as a result of some error before. [Testbot run with this patch](https://testbot.winehq.org/JobDetails.pl?Key=163147) <details> <summary>ASan details</summary> ``` ==setupapi_test.exe==752==ERROR: AddressSanitizer: heap-buffer-overflow on address 0x102d403d8988 at pc 0x6ffffe8067f3 bp 0x7ffffe20e730 sp 0x7ffffe20e778 READ of size 522 at 0x102d403d8988 thread T0 #0 0x6ffffe8067f2 in wcslen /home/runner/work/llvm-mingw/llvm-mingw/llvm-project/compiler-rt/lib/asan/../sanitizer_common/sanitizer_common_interceptors.inc:7258:3 #1 0x6fffff7b3cf1 in lstrlenW /builds/bernhardu/wine/build64/../include/winbase.h:2434:5 #2 0x6fffff7b4845 in WideCharToMultiByte /builds/bernhardu/wine/build64/../dlls/kernelbase/locale.c:7417:30 #3 0x6ffffb64c86a in SetupCopyOEMInfA /builds/bernhardu/wine/build64/../dlls/setupapi/devinst.c:5307:13 #4 0x00014002fbe5 in test_copy_oem_inf /builds/bernhardu/wine/build64/../dlls/setupapi/tests/devinst.c:4731:11 #5 0x000140001271 in func_devinst /builds/bernhardu/wine/build64/../dlls/setupapi/tests/devinst.c:5376:5 #6 0x000140089cc2 in run_test /builds/bernhardu/wine/build64/../include/wine/test.h:780:5 #7 0x0001400896cb in main /builds/bernhardu/wine/build64/../include/wine/test.h:900:12 #8 0x00014008b96a in mainCRTStartup /builds/bernhardu/wine/build64/../dlls/msvcrt/crt_main.c:70:11 #9 0x6fffffc37b64 in BaseThreadInitThunk (C:\windows\system32\kernel32.dll+0x178027b64) #10 0x6fffffdc0ab6 in RtlUserThreadStart (C:\windows\system32\ntdll.dll+0x170050ab6) 0x102d403d8988 is located 0 bytes after 520-byte region [0x102d403d8780,0x102d403d8988) allocated by thread T0 here: #0 0x6ffffe80da21 in malloc /home/runner/work/llvm-mingw/llvm-mingw/llvm-project/compiler-rt/lib/asan/asan_malloc_win.cpp:87:3 #1 0x6ffffb66e823 in MyMalloc /builds/bernhardu/wine/build64/../dlls/setupapi/misc.c:84:12 #2 0x6ffffb64c723 in SetupCopyOEMInfA /builds/bernhardu/wine/build64/../dlls/setupapi/devinst.c:5295:27 #3 0x00014002fbe5 in test_copy_oem_inf /builds/bernhardu/wine/build64/../dlls/setupapi/tests/devinst.c:4731:11 #4 0x000140001271 in func_devinst /builds/bernhardu/wine/build64/../dlls/setupapi/tests/devinst.c:5376:5 #5 0x000140089cc2 in run_test /builds/bernhardu/wine/build64/../include/wine/test.h:780:5 #6 0x0001400896cb in main /builds/bernhardu/wine/build64/../include/wine/test.h:900:12 #7 0x00014008b96a in mainCRTStartup /builds/bernhardu/wine/build64/../dlls/msvcrt/crt_main.c:70:11 #8 0x6fffffc37b64 in BaseThreadInitThunk (C:\windows\system32\kernel32.dll+0x178027b64) #9 0x6fffffdc0ab6 in RtlUserThreadStart (C:\windows\system32\ntdll.dll+0x170050ab6) SUMMARY: AddressSanitizer: heap-buffer-overflow /builds/bernhardu/wine/build64/../include/winbase.h:2434:5 in lstrlenW ``` </details> -- https://gitlab.winehq.org/wine/wine/-/merge_requests/11014
From: Bernhard Übelacker <bernhardu@mailbox.org> Otherwise the call to WideCharToMultiByte does a lstrlenW, which may overrun the uninitialized destW buffer. --- dlls/setupapi/devinst.c | 3 ++- dlls/setupapi/tests/devinst.c | 7 +++++++ 2 files changed, 9 insertions(+), 1 deletion(-) diff --git a/dlls/setupapi/devinst.c b/dlls/setupapi/devinst.c index f6bfcc483b0..744b424eb94 100644 --- a/dlls/setupapi/devinst.c +++ b/dlls/setupapi/devinst.c @@ -5287,12 +5287,13 @@ BOOL WINAPI SetupCopyOEMInfA( PCSTR source, PCSTR location, { BOOL ret = FALSE; LPWSTR destW = NULL, sourceW = NULL, locationW = NULL; - DWORD size; + DWORD size = 0; TRACE("%s, %s, %ld, %ld, %p, %ld, %p, %p\n", debugstr_a(source), debugstr_a(location), media_type, style, dest, buffer_size, required_size, component); if (dest && !(destW = MyMalloc( buffer_size * sizeof(WCHAR) ))) return FALSE; + if (destW) destW[0] = L'\0'; if (source && !(sourceW = strdupAtoW( source ))) goto done; if (location && !(locationW = strdupAtoW( location ))) goto done; diff --git a/dlls/setupapi/tests/devinst.c b/dlls/setupapi/tests/devinst.c index 98cec632219..d21a78b36ad 100644 --- a/dlls/setupapi/tests/devinst.c +++ b/dlls/setupapi/tests/devinst.c @@ -4654,6 +4654,13 @@ static void test_copy_oem_inf(struct testsign_context *ctx) ok(!ret, "Got %d.\n", ret); ok(GetLastError() == ERROR_FILE_NOT_FOUND, "Got error %#lx.\n", GetLastError()); + /* try a relative nonexistent SourceInfFileName, with dest parameter */ + memset(dest, 0xcc, sizeof(dest)); + SetLastError(0xdeadbeef); + ret = SetupCopyOEMInfA("nonexistent", NULL, 0, SP_COPY_NOOVERWRITE, dest, sizeof(dest), NULL, NULL); + ok(!ret, "Got %d.\n", ret); + ok(GetLastError() == ERROR_FILE_NOT_FOUND, "Got error %#lx.\n", GetLastError()); + /* try an absolute nonexistent SourceInfFileName */ GetCurrentDirectoryA(sizeof(path), path); strcat(path, "\\nonexistent"); -- GitLab https://gitlab.winehq.org/wine/wine/-/merge_requests/11014
That doesn't seem like a very declarative solution. Rather we should be checking the return value of SetupCopyOEMInfW(). -- https://gitlab.winehq.org/wine/wine/-/merge_requests/11014#note_141553
On Thu May 28 21:15:48 2026 +0000, Elizabeth Figura wrote:
That doesn't seem like a very declarative solution. Rather we should be checking the return value of SetupCopyOEMInfW(). Thanks for taking a look.
I first thought also at something like the patch in https://testbot.winehq.org/JobDetails.pl?Key=163148 . This seems to [show a situation](https://gitlab.winehq.org/wine/wine/-/blob/master/dlls/setupapi/tests/devins...) where we need to copy destW to dest even when the SetupCopyOEMInfW returned FALSE. Therefore I tried not to touch current conditions around the copy and find a solution without checking the return value. -- https://gitlab.winehq.org/wine/wine/-/merge_requests/11014#note_141571
On Thu May 28 21:15:48 2026 +0000, Bernhard Übelacker wrote:
Thanks for taking a look. I first thought also at something like the patch in https://testbot.winehq.org/JobDetails.pl?Key=163148 . This seems to [show a situation](https://gitlab.winehq.org/wine/wine/-/blob/master/dlls/setupapi/tests/devins...) where we need to copy destW to dest even when the SetupCopyOEMInfW returned FALSE. Therefore I tried not to touch current conditions around the copy and find a solution without checking the return value. I think it would still be better to check for "ret || GetLastError() == ERROR_FILE_EXISTS" explicitly.
-- https://gitlab.winehq.org/wine/wine/-/merge_requests/11014#note_141591
participants (3)
-
Bernhard Übelacker -
Bernhard Übelacker (＠bernhardu)
-
Elizabeth Figura (＠zfigura)