[PATCH 1/7] ntdll: Prevent double free (Coverity)
fd will be closed at the end of the function anyways Signed-off-by: Fabian Maurer <dark.shadow4(a)web.de> --- dlls/ntdll/unix/file.c | 1 - 1 file changed, 1 deletion(-) diff --git a/dlls/ntdll/unix/file.c b/dlls/ntdll/unix/file.c index cc8bf0c6e82..a6d76a49b27 100644 --- a/dlls/ntdll/unix/file.c +++ b/dlls/ntdll/unix/file.c @@ -4215,7 +4215,6 @@ NTSTATUS WINAPI NtQueryInformationFile( HANDLE handle, IO_STATUS_BLOCK *io, int res = recv( fd, tmpbuf, size, MSG_PEEK ); info->MessagesAvailable = (res > 0); info->NextMessageSize = (res >= 0) ? res : MAILSLOT_NO_MESSAGE; - if (needs_close) close( fd ); } free( tmpbuf ); } -- 2.36.0
Signed-off-by: Fabian Maurer <dark.shadow4(a)web.de> --- dlls/printui/printui.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/dlls/printui/printui.c b/dlls/printui/printui.c index 8f65ed27e87..13669f05aab 100644 --- a/dlls/printui/printui.c +++ b/dlls/printui/printui.c @@ -227,7 +227,7 @@ void WINAPI PrintUIEntryW(HWND hWnd, HINSTANCE hInst, LPCWSTR pCommand, DWORD nC cx.nCmdShow = nCmdShow; if ((pCommand) && (pCommand[0])) { - /* result is allocated with GlobalAlloc() */ + /* result is allocated with LocalAlloc() */ cx.argv = CommandLineToArgvW(pCommand, &cx.argc); TRACE("got %d args at %p\n", cx.argc, cx.argv); @@ -253,6 +253,6 @@ void WINAPI PrintUIEntryW(HWND hWnd, HINSTANCE hInst, LPCWSTR pCommand, DWORD nC FIXME("dialog: Printer / The operation was not successful\n"); } - GlobalFree(cx.argv); + LocalFree(cx.argv); } -- 2.36.0
Signed-off-by: Fabian Maurer <dark.shadow4(a)web.de> --- 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 54b72091bf5..1651ca69d29 100644 --- a/dlls/mscoree/corruntimehost.c +++ b/dlls/mscoree/corruntimehost.c @@ -1093,7 +1093,7 @@ static void get_utf8_args(int *argc, char ***argv) (*argv)[*argc] = NULL; - HeapFree(GetProcessHeap(), 0, argvw); + LocalFree(argvw); } #if __i386__ -- 2.36.0
Signed-off-by: Esme Povirk <esme(a)codeweavers.com>
Otherwise when hr is not SUCCEEDED we use array and free it again. Signed-off-by: Fabian Maurer <dark.shadow4(a)web.de> --- dlls/shell32/shellitem.c | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/dlls/shell32/shellitem.c b/dlls/shell32/shellitem.c index 0a3a76cbd6a..8568ec34a83 100644 --- a/dlls/shell32/shellitem.c +++ b/dlls/shell32/shellitem.c @@ -1402,9 +1402,11 @@ HRESULT WINAPI SHCreateShellItemArrayFromIDLists(UINT cidl, if(SUCCEEDED(ret)) { ret = create_shellitemarray(array, cidl, psia); - heap_free(array); if(SUCCEEDED(ret)) + { + heap_free(array); return ret; + } } for(i = 0; i < cidl; i++) -- 2.36.0
Hi, While running your changed tests, I think I found new failures. Being a bot and all I'm not very good at pattern recognition, so I might be wrong, but could you please double-check? Full results can be found at: https://testbot.winehq.org/JobDetails.pl?Key=113224 Your paranoid android. === debian11 (32 bit Chinese:China report) === shell32: shelllink.c:814: Test failed: failed to delete link 'C:\users\winetest\Temp\\test.lnk' (32)
On 4/23/22 21:17, Fabian Maurer wrote:
Otherwise when hr is not SUCCEEDED we use array and free it again.
Signed-off-by: Fabian Maurer <dark.shadow4(a)web.de> --- dlls/shell32/shellitem.c | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-)
diff --git a/dlls/shell32/shellitem.c b/dlls/shell32/shellitem.c index 0a3a76cbd6a..8568ec34a83 100644 --- a/dlls/shell32/shellitem.c +++ b/dlls/shell32/shellitem.c @@ -1402,9 +1402,11 @@ HRESULT WINAPI SHCreateShellItemArrayFromIDLists(UINT cidl, if(SUCCEEDED(ret)) { ret = create_shellitemarray(array, cidl, psia); - heap_free(array); if(SUCCEEDED(ret)) + { + heap_free(array); return ret; + } }
for(i = 0; i < cidl; i++) -- 2.36.0
It would be shorter to release array elements when FAILED(), and have a single call to free pointers array.
--- a/dlls/shell32/shellitem.c +++ b/dlls/shell32/shellitem.c @@ -1402,9 +1402,11 @@ HRESULT WINAPI SHCreateShellItemArrayFromIDLists(UINT cidl,> if(SUCCEEDED(ret)) {
ret = create_shellitemarray(array, cidl, psia);
- heap_free(array);
if(SUCCEEDED(ret))
+ { + heap_free(array);
return ret;
+ }
}
for(i = 0; i < cidl; i++)
-- 2.36.0
It would be shorter to release array elements when FAILED(), and have a single call to free pointers array.
We have a very similar case in SHCreateShellItemArray, want me to rewrite that to use the same approach then? Regards, Fabian Maurer
Later we check if they are set before calling IMFTopologyNode_Release, but they are possibly never initialized Signed-off-by: Fabian Maurer <dark.shadow4(a)web.de> --- dlls/mfplay/player.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/dlls/mfplay/player.c b/dlls/mfplay/player.c index 7d9a4da8649..c8bb04f697e 100644 --- a/dlls/mfplay/player.c +++ b/dlls/mfplay/player.c @@ -1378,7 +1378,7 @@ static HRESULT media_item_create_sink_node(IUnknown *sink, IMFTopologyNode **nod static HRESULT media_item_create_topology(struct media_player *player, struct media_item *item, IMFTopology **out) { - IMFTopologyNode *src_node, *sink_node; + IMFTopologyNode *src_node = 0, *sink_node = 0; BOOL selected, video_added = FALSE; IMFStreamDescriptor *sd; IMFTopology *topology; -- 2.36.0
On 4/23/22 21:17, Fabian Maurer wrote:
Later we check if they are set before calling IMFTopologyNode_Release, but they are possibly never initialized
Signed-off-by: Fabian Maurer <dark.shadow4(a)web.de> --- dlls/mfplay/player.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/dlls/mfplay/player.c b/dlls/mfplay/player.c index 7d9a4da8649..c8bb04f697e 100644 --- a/dlls/mfplay/player.c +++ b/dlls/mfplay/player.c @@ -1378,7 +1378,7 @@ static HRESULT media_item_create_sink_node(IUnknown *sink, IMFTopologyNode **nod
static HRESULT media_item_create_topology(struct media_player *player, struct media_item *item, IMFTopology **out) { - IMFTopologyNode *src_node, *sink_node; + IMFTopologyNode *src_node = 0, *sink_node = 0; BOOL selected, video_added = FALSE; IMFStreamDescriptor *sd; IMFTopology *topology; -- 2.36.0
Yes, this looks good. Let's use NULLs instead of 0s though.
When tmp is freed, dos_name is freed as well. We can't later use it to print a message. Signed-off-by: Fabian Maurer <dark.shadow4(a)web.de> --- dlls/ntdll/unix/loader.c | 15 ++++++++++----- 1 file changed, 10 insertions(+), 5 deletions(-) diff --git a/dlls/ntdll/unix/loader.c b/dlls/ntdll/unix/loader.c index d1c42ddc0f3..e577afdb14b 100644 --- a/dlls/ntdll/unix/loader.c +++ b/dlls/ntdll/unix/loader.c @@ -1826,27 +1826,32 @@ NTSTATUS load_main_exe( const WCHAR *dos_name, const char *unix_name, const WCHA (dos_name[0] && dos_name[1] == ':')); if ((status = get_full_path( dos_name, curdir, image ))) goto failed; - free( tmp ); init_unicode_string( &nt_name, *image ); if (loadorder == LO_INVALID) loadorder = get_load_order( &nt_name ); status = open_main_image( *image, module, &main_image_info, loadorder ); - if (status != STATUS_DLL_NOT_FOUND) return status; + if (status != STATUS_DLL_NOT_FOUND) goto end; /* if path is in system dir, we can load the builtin even if the file itself doesn't exist */ if (loadorder != LO_NATIVE && is_builtin_path( &nt_name, &machine )) { status = find_builtin_dll( &nt_name, module, &size, &main_image_info, machine, FALSE ); - if (status != STATUS_DLL_NOT_FOUND) return status; + if (status != STATUS_DLL_NOT_FOUND) goto end; + } + if (!contains_path) + { + status = STATUS_DLL_NOT_FOUND; + goto end; } - if (!contains_path) return STATUS_DLL_NOT_FOUND; failed: MESSAGE( "wine: failed to open %s: %x\n", unix_name ? debugstr_a(unix_name) : debugstr_w(dos_name), status ); NtTerminateProcess( GetCurrentProcess(), status ); - return status; /* unreached */ +end: + free(tmp); + return status; } -- 2.36.0
Fabian Maurer <dark.shadow4(a)web.de> writes:
When tmp is freed, dos_name is freed as well. We can't later use it to print a message.
It's only used if there's no unix name, in which case it wasn't allocated or freed. -- Alexandre Julliard julliard(a)winehq.org
On Montag, 25. April 2022 11:35:51 CEST you wrote:
Fabian Maurer <dark.shadow4(a)web.de> writes:
When tmp is freed, dos_name is freed as well. We can't later use it to print a message.
It's only used if there's no unix name, in which case it wasn't allocated or freed.
Sorry, false positive. Regards, Fabian Maurer
Signed-off-by: Fabian Maurer <dark.shadow4(a)web.de> --- dlls/mfplat/mediatype.c | 1 + 1 file changed, 1 insertion(+) diff --git a/dlls/mfplat/mediatype.c b/dlls/mfplat/mediatype.c index 71d55e2d232..a05fec9ad43 100644 --- a/dlls/mfplat/mediatype.c +++ b/dlls/mfplat/mediatype.c @@ -1380,6 +1380,7 @@ static const WAVEFORMATEX * WINAPI audio_mediatype_GetAudioFormat(IMFAudioMediaT &size, MFWaveFormatExConvertFlag_Normal))) { WARN("Failed to create wave format description, hr %#lx.\n", hr); + media_type->audio_format = 0; } return media_type->audio_format; -- 2.36.0
On 4/23/22 21:17, Fabian Maurer wrote:
Signed-off-by: Fabian Maurer <dark.shadow4(a)web.de> --- dlls/mfplat/mediatype.c | 1 + 1 file changed, 1 insertion(+)
diff --git a/dlls/mfplat/mediatype.c b/dlls/mfplat/mediatype.c index 71d55e2d232..a05fec9ad43 100644 --- a/dlls/mfplat/mediatype.c +++ b/dlls/mfplat/mediatype.c @@ -1380,6 +1380,7 @@ static const WAVEFORMATEX * WINAPI audio_mediatype_GetAudioFormat(IMFAudioMediaT &size, MFWaveFormatExConvertFlag_Normal))) { WARN("Failed to create wave format description, hr %#lx.\n", hr); + media_type->audio_format = 0; }
This makes sense, but let's reset it after CoTaskMemFree(), and use NULL as it's more common for null pointers. Same issue with GetVideoFormat().
On Samstag, 23. April 2022 23:19:14 CEST Nikolay Sivov wrote:
On 4/23/22 21:17, Fabian Maurer wrote:
Signed-off-by: Fabian Maurer <dark.shadow4(a)web.de> ---
dlls/mfplat/mediatype.c | 1 + 1 file changed, 1 insertion(+)
diff --git a/dlls/mfplat/mediatype.c b/dlls/mfplat/mediatype.c index 71d55e2d232..a05fec9ad43 100644 --- a/dlls/mfplat/mediatype.c +++ b/dlls/mfplat/mediatype.c @@ -1380,6 +1380,7 @@ static const WAVEFORMATEX * WINAPI audio_mediatype_GetAudioFormat(IMFAudioMediaT> &size, MFWaveFormatExConvertFlag_Normal)))
{
WARN("Failed to create wave format description, hr %#lx.\n", hr);
+ media_type->audio_format = 0;
}
This makes sense, but let's reset it after CoTaskMemFree(), and use NULL as it's more common for null pointers. Same issue with GetVideoFormat().
Thanks for your comments, I'll resend soon. Regards, Fabian Maurer
participants (5)
-
Alexandre Julliard -
Esme Povirk (she/they) -
Fabian Maurer -
Marvin -
Nikolay Sivov