[PATCH v3 0/9] MR180: misc: Fixes and workarounds for UAF warnings. (GCC12)
The mscoree one is a genuine UAF bug, other are more arguably false positive but GCC doesn't like freed pointer value to be used in any way. -- v3: server: Avoid using pointer value after realloc. d3drm: Avoid using pointer value after free. dsound: Avoid using pointer value after free. notepad: Avoid using pointer value after free. msi: Avoid using pointer value after free. user32/tests: Workaround use after free warnings. oleaut32/tests: Workaround use after free warnings. mshtml/tests: Avoid using pointer value after free. mscoree: Avoid using pointer after free. https://gitlab.winehq.org/wine/wine/-/merge_requests/180
From: Rémi Bernon <rbernon(a)codeweavers.com> Signed-off-by: Rémi Bernon <rbernon(a)codeweavers.com> --- dlls/mscoree/corruntimehost.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/dlls/mscoree/corruntimehost.c b/dlls/mscoree/corruntimehost.c index 1651ca69d29..42081ad99bb 100644 --- a/dlls/mscoree/corruntimehost.c +++ b/dlls/mscoree/corruntimehost.c @@ -1917,7 +1917,7 @@ HRESULT create_monodata(REFCLSID clsid, LPVOID *ppObj) HeapFree(GetProcessHeap(), 0, filenameA); if (!assembly) { - ERR("Cannot open assembly %s, status=%i\n", filenameA, status); + ERR("Cannot open assembly %s, status=%i\n", debugstr_w(filename), status); domain_restore(prev_domain); goto cleanup; } -- GitLab https://gitlab.winehq.org/wine/wine/-/merge_requests/180
This merge request was approved by Alexandre Julliard. -- https://gitlab.winehq.org/wine/wine/-/merge_requests/180
From: Rémi Bernon <rbernon(a)codeweavers.com> Signed-off-by: Rémi Bernon <rbernon(a)codeweavers.com> --- dlls/mshtml/tests/misc.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/dlls/mshtml/tests/misc.c b/dlls/mshtml/tests/misc.c index 410d24884ee..72692c4915b 100644 --- a/dlls/mshtml/tests/misc.c +++ b/dlls/mshtml/tests/misc.c @@ -240,13 +240,13 @@ static void test_HTMLStorage(void) hres = IHTMLStorage_getItem(storage, key, &var); ok(hres == S_OK, "getItem failed: %08lx\n", hres); ok(V_VT(&var) == VT_NULL, "got %d\n", V_VT(&var)); - SysFreeString(key); hres = IHTMLStorage_getItem(storage, NULL, NULL); ok(hres == E_POINTER, "getItem returned: %08lx\n", hres); hres = IHTMLStorage_getItem(storage, key, NULL); ok(hres == E_POINTER, "getItem failed: %08lx\n", hres); + SysFreeString(key); V_VT(&var) = 0xdead; hres = IHTMLStorage_getItem(storage, NULL, &var); -- GitLab https://gitlab.winehq.org/wine/wine/-/merge_requests/180
From: Rémi Bernon <rbernon(a)codeweavers.com> Signed-off-by: Rémi Bernon <rbernon(a)codeweavers.com> --- dlls/oleaut32/tests/vartype.c | 11 +++++++---- 1 file changed, 7 insertions(+), 4 deletions(-) diff --git a/dlls/oleaut32/tests/vartype.c b/dlls/oleaut32/tests/vartype.c index c5137572d9d..d955a6d86da 100644 --- a/dlls/oleaut32/tests/vartype.c +++ b/dlls/oleaut32/tests/vartype.c @@ -31,6 +31,8 @@ DEFINE_GUID(UUID_test_struct, 0x4029f190, 0xca4a, 0x4611, 0xae,0xb9,0x67,0x39,0x83,0xcb,0x96,0xdd); +static void (WINAPI *pSysFreeString)(BSTR); + /* Some Visual C++ versions choke on __uint64 to float conversions. * To fix this you need either VC++ 6.0 plus the processor pack * or Visual C++ >=7.0. @@ -5843,9 +5845,9 @@ static void test_bstr_cache(void) str = SysAllocString(testW); /* This should put the string into cache */ - SysFreeString(str); + pSysFreeString(str); /* The string is in cache, this won't touch it */ - SysFreeString(str); + pSysFreeString(str); ok(SysStringLen(str) == 4, "unexpected len\n"); ok(!lstrcmpW(str, testW), "string changed\n"); @@ -5874,8 +5876,8 @@ static void test_bstr_cache(void) str2 = SysAllocStringLen(NULL, 16); ok(str2 == strs[1], "str2 != strs[1]\n"); - SysFreeString(str); - SysFreeString(str2); + pSysFreeString(str); + pSysFreeString(str2); SysFreeString(str); SysFreeString(str2); } @@ -6047,6 +6049,7 @@ static void test_recinfo(void) START_TEST(vartype) { hOleaut32 = GetModuleHandleA("oleaut32.dll"); + pSysFreeString = (void *)GetProcAddress(hOleaut32, "SysFreeString"); has_i8 = GetProcAddress(hOleaut32, "VarI8FromI1") != NULL; has_locales = has_i8 && GetProcAddress(hOleaut32, "GetVarConversionLocaleSetting") != NULL; -- GitLab https://gitlab.winehq.org/wine/wine/-/merge_requests/180
From: Rémi Bernon <rbernon(a)codeweavers.com> Signed-off-by: Rémi Bernon <rbernon(a)codeweavers.com> --- dlls/user32/tests/clipboard.c | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/dlls/user32/tests/clipboard.c b/dlls/user32/tests/clipboard.c index e05de29805e..3131c2e0e8c 100644 --- a/dlls/user32/tests/clipboard.c +++ b/dlls/user32/tests/clipboard.c @@ -29,6 +29,7 @@ static BOOL (WINAPI *pAddClipboardFormatListener)(HWND hwnd); static BOOL (WINAPI *pRemoveClipboardFormatListener)(HWND hwnd); static BOOL (WINAPI *pGetUpdatedClipboardFormats)( UINT *formats, UINT count, UINT *out_count ); +static HGLOBAL (WINAPI *pGlobalFree)(HGLOBAL); static int thread_from_line; static char *argv0; @@ -2073,7 +2074,7 @@ static void test_data_handles(void) metafile = create_metafile(); h = SetClipboardData( CF_METAFILEPICT, metafile ); ok( h == metafile, "Expected metafilepict %p, got %p.\n", metafile, h ); - ok( !GlobalFree( metafile ), "GlobalFree failed.\n" ); + ok( !pGlobalFree( metafile ), "GlobalFree failed.\n" ); h = GetClipboardData( CF_METAFILEPICT ); ok( h == metafile, "Expected metafile %p, got %p.\n", metafile, h ); ok( is_freed( h ), "Expected freed mem %p.\n", h ); @@ -2081,7 +2082,7 @@ static void test_data_handles(void) metafile = create_metafile(); h = SetClipboardData( CF_DSPMETAFILEPICT, metafile ); ok( h == metafile, "Expected metafilepict %p, got %p.\n", metafile, h ); - ok( !GlobalFree( metafile ), "GlobalFree failed.\n" ); + ok( !pGlobalFree( metafile ), "GlobalFree failed.\n" ); h = GetClipboardData( CF_DSPMETAFILEPICT ); ok( h == metafile, "Expected metafile %p, got %p.\n", metafile, h ); ok( is_freed( h ), "Expected freed mem %p.\n", h ); @@ -2343,6 +2344,7 @@ START_TEST(clipboard) pAddClipboardFormatListener = (void *)GetProcAddress( mod, "AddClipboardFormatListener" ); pRemoveClipboardFormatListener = (void *)GetProcAddress( mod, "RemoveClipboardFormatListener" ); pGetUpdatedClipboardFormats = (void *)GetProcAddress( mod, "GetUpdatedClipboardFormats" ); + pGlobalFree = (void *)GetProcAddress( GetModuleHandleA( "kernel32" ), "GlobalFree" ); if (argc == 4 && !strcmp( argv[2], "set_clipboard_data" )) { -- GitLab https://gitlab.winehq.org/wine/wine/-/merge_requests/180
From: Rémi Bernon <rbernon(a)codeweavers.com> Signed-off-by: Rémi Bernon <rbernon(a)codeweavers.com> --- dlls/msi/handle.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/dlls/msi/handle.c b/dlls/msi/handle.c index 7be64da9bfe..f0a55956fce 100644 --- a/dlls/msi/handle.c +++ b/dlls/msi/handle.c @@ -256,8 +256,8 @@ int msiobj_release( MSIOBJECTHDR *info ) { if( info->destructor ) info->destructor( info ); - msi_free( info ); TRACE("object %p destroyed\n", info); + msi_free( info ); } return ret; -- GitLab https://gitlab.winehq.org/wine/wine/-/merge_requests/180
From: Rémi Bernon <rbernon(a)codeweavers.com> Signed-off-by: Rémi Bernon <rbernon(a)codeweavers.com> --- programs/notepad/main.c | 32 ++++++++++++++++++-------------- 1 file changed, 18 insertions(+), 14 deletions(-) diff --git a/programs/notepad/main.c b/programs/notepad/main.c index 210ecd2bfc8..b1cdad8156d 100644 --- a/programs/notepad/main.c +++ b/programs/notepad/main.c @@ -377,10 +377,9 @@ static LPWSTR NOTEPAD_StrRStr(LPWSTR pszSource, LPWSTR pszLast, LPWSTR pszSrch) void NOTEPAD_DoFind(FINDREPLACEW *fr) { LPWSTR content; - LPWSTR found; int len = lstrlenW(fr->lpstrFindWhat); int fileLen; - DWORD pos; + SIZE_T pos; fileLen = GetWindowTextLengthW(Globals.hEdit) + 1; content = HeapAlloc(GetProcessHeap(), 0, fileLen * sizeof(WCHAR)); @@ -391,30 +390,34 @@ void NOTEPAD_DoFind(FINDREPLACEW *fr) switch (fr->Flags & (FR_DOWN|FR_MATCHCASE)) { case 0: - found = StrRStrIW(content, content+pos-len, fr->lpstrFindWhat); + pos = StrRStrIW(content, content+pos-len, fr->lpstrFindWhat) - content; + if (pos == -(SIZE_T)content) pos = ~(SIZE_T)0; break; case FR_DOWN: - found = StrStrIW(content+pos, fr->lpstrFindWhat); + pos = StrStrIW(content+pos, fr->lpstrFindWhat) - content; + if (pos == -(SIZE_T)content) pos = ~(SIZE_T)0; break; case FR_MATCHCASE: - found = NOTEPAD_StrRStr(content, content+pos-len, fr->lpstrFindWhat); + pos = NOTEPAD_StrRStr(content, content+pos-len, fr->lpstrFindWhat) - content; + if (pos == -(SIZE_T)content) pos = ~(SIZE_T)0; break; case FR_DOWN|FR_MATCHCASE: - found = StrStrW(content+pos, fr->lpstrFindWhat); + pos = StrStrW(content+pos, fr->lpstrFindWhat) - content; + if (pos == -(SIZE_T)content) pos = ~(SIZE_T)0; break; default: /* shouldn't happen */ return; } HeapFree(GetProcessHeap(), 0, content); - if (found == NULL) + if (pos == ~(SIZE_T)0) { DIALOG_StringMsgBox(Globals.hFindReplaceDlg, STRING_NOTFOUND, fr->lpstrFindWhat, MB_ICONINFORMATION|MB_OK); return; } - SendMessageW(Globals.hEdit, EM_SETSEL, found - content, found - content + len); + SendMessageW(Globals.hEdit, EM_SETSEL, pos, pos + len); } static void NOTEPAD_DoReplace(FINDREPLACEW *fr) @@ -452,10 +455,9 @@ static void NOTEPAD_DoReplace(FINDREPLACEW *fr) static void NOTEPAD_DoReplaceAll(FINDREPLACEW *fr) { LPWSTR content; - LPWSTR found; int len = lstrlenW(fr->lpstrFindWhat); int fileLen; - DWORD pos; + SIZE_T pos; SendMessageW(Globals.hEdit, EM_SETSEL, 0, 0); while(TRUE){ @@ -468,22 +470,24 @@ static void NOTEPAD_DoReplaceAll(FINDREPLACEW *fr) switch (fr->Flags & (FR_DOWN|FR_MATCHCASE)) { case FR_DOWN: - found = StrStrIW(content+pos, fr->lpstrFindWhat); + pos = StrStrIW(content+pos, fr->lpstrFindWhat) - content; + if (pos == -(SIZE_T)content) pos = ~(SIZE_T)0; break; case FR_DOWN|FR_MATCHCASE: - found = StrStrW(content+pos, fr->lpstrFindWhat); + pos = StrStrW(content+pos, fr->lpstrFindWhat) - content; + if (pos == -(SIZE_T)content) pos = ~(SIZE_T)0; break; default: /* shouldn't happen */ return; } HeapFree(GetProcessHeap(), 0, content); - if(found == NULL) + if(pos == ~(SIZE_T)0) { SendMessageW(Globals.hEdit, EM_SETSEL, 0, 0); return; } - SendMessageW(Globals.hEdit, EM_SETSEL, found - content, found - content + len); + SendMessageW(Globals.hEdit, EM_SETSEL, pos, pos + len); SendMessageW(Globals.hEdit, EM_REPLACESEL, TRUE, (LPARAM)fr->lpstrReplaceWith); } } -- GitLab https://gitlab.winehq.org/wine/wine/-/merge_requests/180
From: Rémi Bernon <rbernon(a)codeweavers.com> Signed-off-by: Rémi Bernon <rbernon(a)codeweavers.com> --- dlls/dsound/buffer.c | 4 ++-- dlls/dsound/capture.c | 6 +++--- dlls/dsound/dsound.c | 4 ++-- dlls/dsound/duplex.c | 2 +- dlls/dsound/propset.c | 2 +- 5 files changed, 9 insertions(+), 9 deletions(-) diff --git a/dlls/dsound/buffer.c b/dlls/dsound/buffer.c index eb87479b7df..799b64e0e38 100644 --- a/dlls/dsound/buffer.c +++ b/dlls/dsound/buffer.c @@ -1192,9 +1192,9 @@ void secondarybuffer_destroy(IDirectSoundBufferImpl *This) HeapFree(GetProcessHeap(), 0, This->filters); } - HeapFree(GetProcessHeap(), 0, This); - TRACE("(%p) released\n", This); + + HeapFree(GetProcessHeap(), 0, This); } BOOL secondarybuffer_is_audible(IDirectSoundBufferImpl *This) diff --git a/dlls/dsound/capture.c b/dlls/dsound/capture.c index a00748d95bf..2c3dd83e86d 100644 --- a/dlls/dsound/capture.c +++ b/dlls/dsound/capture.c @@ -109,8 +109,8 @@ static void capturebuffer_destroy(IDirectSoundCaptureBufferImpl *This) This->device->capture_buffer = NULL; HeapFree(GetProcessHeap(), 0, This->notifies); - HeapFree(GetProcessHeap(), 0, This); TRACE("(%p) released\n", This); + HeapFree(GetProcessHeap(), 0, This); } /******************************************************************************* @@ -871,8 +871,8 @@ static ULONG DirectSoundCaptureDevice_Release( HeapFree(GetProcessHeap(), 0, device->pwfx); device->lock.DebugInfo->Spare[0] = 0; DeleteCriticalSection( &(device->lock) ); + TRACE("(%p) released\n", device); HeapFree(GetProcessHeap(), 0, device); - TRACE("(%p) released\n", device); } return ref; } @@ -1099,8 +1099,8 @@ static void capture_destroy(IDirectSoundCaptureImpl *This) { if (This->device) DirectSoundCaptureDevice_Release(This->device); - HeapFree(GetProcessHeap(),0,This); TRACE("(%p) released\n", This); + HeapFree(GetProcessHeap(),0,This); } /******************************************************************************* diff --git a/dlls/dsound/dsound.c b/dlls/dsound/dsound.c index 346cc4ceca3..f2aea64eeed 100644 --- a/dlls/dsound/dsound.c +++ b/dlls/dsound/dsound.c @@ -239,8 +239,8 @@ static ULONG DirectSoundDevice_Release(DirectSoundDevice * device) HeapFree(GetProcessHeap(), 0, device->buffer); device->mixlock.DebugInfo->Spare[0] = 0; DeleteCriticalSection(&device->mixlock); - HeapFree(GetProcessHeap(),0,device); TRACE("(%p) released\n", device); + HeapFree(GetProcessHeap(),0,device); } return ref; } @@ -670,8 +670,8 @@ static void directsound_destroy(IDirectSoundImpl *This) { if (This->device) DirectSoundDevice_Release(This->device); - HeapFree(GetProcessHeap(),0,This); TRACE("(%p) released\n", This); + HeapFree(GetProcessHeap(),0,This); } static inline IDirectSoundImpl *impl_from_IUnknown(IUnknown *iface) diff --git a/dlls/dsound/duplex.c b/dlls/dsound/duplex.c index a3a9a066c31..b7891caa410 100644 --- a/dlls/dsound/duplex.c +++ b/dlls/dsound/duplex.c @@ -61,8 +61,8 @@ static void fullduplex_destroy(IDirectSoundFullDuplexImpl *This) while(IDirectSoundCapture_Release(dsc8) > 0); IUnknown_Release(This->dsc8_unk); } - HeapFree(GetProcessHeap(), 0, This); TRACE("(%p) released\n", This); + HeapFree(GetProcessHeap(), 0, This); } /******************************************************************************* diff --git a/dlls/dsound/propset.c b/dlls/dsound/propset.c index 74d66cad68e..f42cfbf4163 100644 --- a/dlls/dsound/propset.c +++ b/dlls/dsound/propset.c @@ -91,8 +91,8 @@ static ULONG WINAPI IKsPrivatePropertySetImpl_Release(LPKSPROPERTYSET iface) TRACE("(%p) ref %ld\n", This, ref); if (!ref) { + TRACE("(%p) released\n", This); HeapFree(GetProcessHeap(), 0, This); - TRACE("(%p) released\n", This); } return ref; } -- GitLab https://gitlab.winehq.org/wine/wine/-/merge_requests/180
From: Rémi Bernon <rbernon(a)codeweavers.com> Signed-off-by: Rémi Bernon <rbernon(a)codeweavers.com> --- dlls/d3drm/d3drm.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/dlls/d3drm/d3drm.c b/dlls/d3drm/d3drm.c index 864323053f3..cc83ae6d1f3 100644 --- a/dlls/d3drm/d3drm.c +++ b/dlls/d3drm/d3drm.c @@ -210,8 +210,8 @@ static inline struct d3drm *impl_from_IDirect3DRM3(IDirect3DRM3 *iface) static void d3drm_destroy(struct d3drm *d3drm) { - heap_free(d3drm); TRACE("d3drm object %p is being destroyed.\n", d3drm); + heap_free(d3drm); } static HRESULT WINAPI d3drm1_QueryInterface(IDirect3DRM *iface, REFIID riid, void **out) -- GitLab https://gitlab.winehq.org/wine/wine/-/merge_requests/180
From: Rémi Bernon <rbernon(a)codeweavers.com> Signed-off-by: Rémi Bernon <rbernon(a)codeweavers.com> Signed-off-by: Alexandre Julliard <julliard(a)winehq.org> --- server/queue.c | 17 ++++++++--------- 1 file changed, 8 insertions(+), 9 deletions(-) diff --git a/server/queue.c b/server/queue.c index d79add56fba..934cf358059 100644 --- a/server/queue.c +++ b/server/queue.c @@ -3348,15 +3348,14 @@ DECL_HANDLER(get_cursor_history) DECL_HANDLER(get_rawinput_buffer) { struct thread_input *input = current->queue->input; - data_size_t size = 0, next_size = 0; + data_size_t size = 0, next_size = 0, pos = 0; struct list *ptr; - char *buf, *cur, *tmp; + char *buf, *tmp; int count = 0, buf_size = 16 * sizeof(struct hardware_msg_data); if (!req->buffer_size) buf = NULL; else if (!(buf = mem_alloc( buf_size ))) return; - cur = buf; ptr = list_head( &input->msg_list ); while (ptr) { @@ -3369,31 +3368,31 @@ DECL_HANDLER(get_rawinput_buffer) next_size = req->rawinput_size + extra_size; if (size + next_size > req->buffer_size) break; - if (cur + data->size > buf + get_reply_max_size()) break; - if (cur + data->size > buf + buf_size) + if (pos + data->size > get_reply_max_size()) break; + if (pos + data->size > buf_size) { buf_size += buf_size / 2 + extra_size; if (!(tmp = realloc( buf, buf_size ))) { + free( buf ); set_error( STATUS_NO_MEMORY ); return; } - cur = tmp + (cur - buf); buf = tmp; } - memcpy( cur, data, data->size ); + memcpy( buf + pos, data, data->size ); list_remove( &msg->entry ); free_message( msg ); size += next_size; - cur += sizeof(*data); + pos += sizeof(*data); count++; } reply->next_size = next_size; reply->count = count; - set_reply_data_ptr( buf, cur - buf ); + set_reply_data_ptr( buf, pos ); } DECL_HANDLER(update_rawinput_devices) -- GitLab https://gitlab.winehq.org/wine/wine/-/merge_requests/180
participants (2)
-
Alexandre Julliard (＠julliard) -
Rémi Bernon