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.
From: Rémi Bernon rbernon@codeweavers.com
Signed-off-by: Rémi Bernon rbernon@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; }
From: Rémi Bernon rbernon@codeweavers.com
Signed-off-by: Rémi Bernon rbernon@codeweavers.com --- dlls/mshtml/tests/misc.c | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-)
diff --git a/dlls/mshtml/tests/misc.c b/dlls/mshtml/tests/misc.c index 410d24884ee..80d5283c275 100644 --- a/dlls/mshtml/tests/misc.c +++ b/dlls/mshtml/tests/misc.c @@ -28,6 +28,8 @@ #include "initguid.h" #include "optary.h"
+static void (WINAPI *pSysFreeString)(BSTR); + static void test_HTMLLoadOptions(void) { IHtmlLoadOptions *loadopts; @@ -240,7 +242,7 @@ 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); + pSysFreeString(key);
hres = IHTMLStorage_getItem(storage, NULL, NULL); ok(hres == E_POINTER, "getItem returned: %08lx\n", hres); @@ -298,6 +300,7 @@ static void test_HTMLStorage(void) START_TEST(misc) { CoInitialize(NULL); + pSysFreeString = (void *)GetProcAddress(GetModuleHandleA("oleaut32.dll"), "SysFreeString");
test_HTMLLoadOptions(); test_HTMLStorage();
From: Rémi Bernon rbernon@codeweavers.com
Signed-off-by: Rémi Bernon rbernon@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;
From: Rémi Bernon rbernon@codeweavers.com
Signed-off-by: Rémi Bernon rbernon@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" )) {
From: Rémi Bernon rbernon@codeweavers.com
Signed-off-by: Rémi Bernon rbernon@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;
From: Rémi Bernon rbernon@codeweavers.com
Signed-off-by: Rémi Bernon rbernon@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); } }
From: Rémi Bernon rbernon@codeweavers.com
Signed-off-by: Rémi Bernon rbernon@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; }
From: Rémi Bernon rbernon@codeweavers.com
Signed-off-by: Rémi Bernon rbernon@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)
From: Rémi Bernon rbernon@codeweavers.com
Signed-off-by: Rémi Bernon rbernon@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..e40f5f1f2e3 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; @@ -3373,13 +3373,13 @@ DECL_HANDLER(get_rawinput_buffer) if (cur + data->size > buf + buf_size) { buf_size += buf_size / 2 + extra_size; - if (!(tmp = realloc( buf, buf_size ))) + cur = (char *)(cur - buf); + if (!(buf = realloc( buf, buf_size ))) { set_error( STATUS_NO_MEMORY ); return; } - cur = tmp + (cur - buf); - buf = tmp; + cur = buf + (size_t)cur; }
memcpy( cur, data, data->size );
Rémi Bernon wine@gitlab.winehq.org wrote:
- char *buf, *cur, *tmp;
char *buf, *cur; int count = 0, buf_size = 16 * sizeof(struct hardware_msg_data);
if (!req->buffer_size) buf = NULL;
@@ -3373,13 +3373,13 @@ DECL_HANDLER(get_rawinput_buffer) if (cur + data->size > buf + buf_size) { buf_size += buf_size / 2 + extra_size;
if (!(tmp = realloc( buf, buf_size )))
cur = (char *)(cur - buf);if (!(buf = realloc( buf, buf_size ))) { set_error( STATUS_NO_MEMORY ); return; }
cur = tmp + (cur - buf);buf = tmp;
cur = buf + (size_t)cur; }
Reusing 'cur' as an offset doesn't look very elegant to me. Perhaps a new variable to hold the offset could be more appropriate here?
On 6/3/22 11:39, Dmitry Timoshkov wrote:
Rémi Bernon wine@gitlab.winehq.org wrote:
- char *buf, *cur, *tmp;
char *buf, *cur; int count = 0, buf_size = 16 * sizeof(struct hardware_msg_data);
if (!req->buffer_size) buf = NULL;
@@ -3373,13 +3373,13 @@ DECL_HANDLER(get_rawinput_buffer) if (cur + data->size > buf + buf_size) { buf_size += buf_size / 2 + extra_size;
if (!(tmp = realloc( buf, buf_size )))
cur = (char *)(cur - buf);if (!(buf = realloc( buf, buf_size ))) { set_error( STATUS_NO_MEMORY ); return; }
cur = tmp + (cur - buf);buf = tmp;
cur = buf + (size_t)cur; }Reusing 'cur' as an offset doesn't look very elegant to me. Perhaps a new variable to hold the offset could be more appropriate here?
I actually agree, I considered renaming the variable to "pos", which could maybe do better as both a position or an offset in the buffer, but maybe changing the code to use buf + offset everywhere would be better.
It was a bigger diff though, so it made the change less obvious.
This merge request was approved by Andrew Eikum.
Hans Leidekker (@hans) commented about dlls/mshtml/tests/misc.c:
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);
- pSysFreeString(key);
This SysFreeString() call can be removed instead.
On Fri Jun 3 09:56:05 2022 +0000, **** wrote:
=?UTF-8?Q?R=c3=a9mi_Bernon?= replied on the mailing list:
On 6/3/22 11:39, Dmitry Timoshkov wrote: > Rémi Bernon <wine@gitlab.winehq.org> wrote: > >> - char *buf, *cur, *tmp; >> + char *buf, *cur; >> int count = 0, buf_size = 16 * sizeof(struct hardware_msg_data); >> >> if (!req->buffer_size) buf = NULL; >> @@ -3373,13 +3373,13 @@ DECL_HANDLER(get_rawinput_buffer) >> if (cur + data->size > buf + buf_size) >> { >> buf_size += buf_size / 2 + extra_size; >> - if (!(tmp = realloc( buf, buf_size ))) >> + cur = (char *)(cur - buf); >> + if (!(buf = realloc( buf, buf_size ))) >> { >> set_error( STATUS_NO_MEMORY ); >> return; >> } >> - cur = tmp + (cur - buf); >> - buf = tmp; >> + cur = buf + (size_t)cur; >> } > > Reusing 'cur' as an offset doesn't look very elegant to me. Perhaps > a new variable to hold the offset could be more appropriate here? > I actually agree, I considered renaming the variable to "pos", which could maybe do better as both a position or an offset in the buffer, but maybe changing the code to use buf + offset everywhere would be better. It was a bigger diff though, so it made the change less obvious. -- Rémi Bernon <rbernon@codeweavers.com>
FWIW, I would also lean towards a dedicated variable with the offset, limited to inner scope for clarity. I don't think there's reasons to skimp on variables much these days..?
This merge request was approved by Jan Sikorski.
On Fri Jun 3 13:25:57 2022 +0000, Hans Leidekker wrote:
This SysFreeString() call can be removed instead.
I suppose you mean moved below the problematic `IHTMLStorage_getItem` test maybe? I wasn't sure if that second test intended to use key on purpose, but on a second look it seems to intend to test `NULL` out parameter.
Normally I would sign-off the mscoree patch but I don't know how to do that here.
On Fri Jun 3 15:57:12 2022 +0000, Rémi Bernon wrote:
I suppose you mean moved below the problematic `IHTMLStorage_getItem` test maybe? I wasn't sure if that second test intended to use key on purpose, but on a second look it seems to intend to test `NULL` out parameter.
You're right, 'key' should be freed after that test.
-
Andrew Eikum (＠aeikum) -
Dmitry Timoshkov -
Esme (＠madewokherd)
-
Hans Leidekker (＠hans)
-
Jan Sikorski (＠jsikorski)
-
Rémi Bernon
-
Rémi Bernon -
Rémi Bernon (＠rbernon)