[PATCH v2 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. -- v2: 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
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> --- server/queue.c | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/server/queue.c b/server/queue.c index d79add56fba..9c5469292fe 100644 --- a/server/queue.c +++ b/server/queue.c @@ -3350,7 +3350,7 @@ DECL_HANDLER(get_rawinput_buffer) struct thread_input *input = current->queue->input; data_size_t size = 0, next_size = 0; struct list *ptr; - char *buf, *cur, *tmp; + char *buf, *cur; int count = 0, buf_size = 16 * sizeof(struct hardware_msg_data); if (!req->buffer_size) buf = NULL; @@ -3372,14 +3372,14 @@ DECL_HANDLER(get_rawinput_buffer) if (cur + data->size > buf + get_reply_max_size()) break; if (cur + data->size > buf + buf_size) { + size_t pos = cur - buf; buf_size += buf_size / 2 + extra_size; - if (!(tmp = realloc( buf, buf_size ))) + if (!(buf = realloc( buf, buf_size ))) { set_error( STATUS_NO_MEMORY ); return; } - cur = tmp + (cur - buf); - buf = tmp; + cur = buf + pos; } memcpy( cur, data, data->size ); -- GitLab https://gitlab.winehq.org/wine/wine/-/merge_requests/180
On Mon Jun 6 06:09:19 2022 +0000, Esme wrote:
Normally I would sign-off the mscoree patch but I don't know how to do that here. You're expected to approve the MR if you're okay with the changes, but I see you're not in the reviewers so I think you need to first request project access on https://gitlab.winehq.org/wine/wine to get the permission to review.
-- https://gitlab.winehq.org/wine/wine/-/merge_requests/180#note_1598
This merge request was approved by Hans Leidekker. -- https://gitlab.winehq.org/wine/wine/-/merge_requests/180
On Mon Jun 6 06:09:19 2022 +0000, Rémi Bernon wrote:
You're expected to approve the MR if you're okay with the changes, but I see you're not in the reviewers so I think you need to first request project access on https://gitlab.winehq.org/wine/wine to get the permission to review. Also if you don't mind, could you please use your full name in your gitlab profile? This is used to match with the MAINTAINERS file so that you can be assigned as reviewer automatically.
-- https://gitlab.winehq.org/wine/wine/-/merge_requests/180#note_1600
This merge request was approved by Jacek Caban. -- https://gitlab.winehq.org/wine/wine/-/merge_requests/180
participants (4)
-
Alexandre Julliard (＠julliard) -
Hans Leidekker (＠hans)
-
Jacek Caban (＠jacek)
-
Rémi Bernon