On 18.08.2016 06:37, Alex Henrie wrote:
> Cc: Sebastian Lackner <sebastian(a)fds-team.de>
>
> v7 fixes the use-after-free of V_BSTR(&var).
>
> Signed-off-by: Alex Henrie <alexhenrie24(a)gmail.com>
> ---
> dlls/shell32/tests/Makefile.in | 2 +-
> dlls/shell32/tests/shelldispatch.c | 220 +++++++++++++++++++++++++++++++++++--
> 2 files changed, 210 insertions(+), 12 deletions(-)
>
> diff --git a/dlls/shell32/tests/Makefile.in b/dlls/shell32/tests/Makefile.in
> index cfe0c50..ef95b52 100644
> --- a/dlls/shell32/tests/Makefile.in
> +++ b/dlls/shell32/tests/Makefile.in
> @@ -1,5 +1,5 @@
> TESTDLL = shell32.dll
> -IMPORTS = shell32 ole32 oleaut32 user32 advapi32
> +IMPORTS = shell32 ole32 oleaut32 user32 advapi32 shlwapi
>
> C_SRCS = \
> appbar.c \
> diff --git a/dlls/shell32/tests/shelldispatch.c b/dlls/shell32/tests/shelldispatch.c
> index 9a47c67..02be8fd 100644
> --- a/dlls/shell32/tests/shelldispatch.c
> +++ b/dlls/shell32/tests/shelldispatch.c
> @@ -25,13 +25,14 @@
> #include "shldisp.h"
> #include "shlobj.h"
> #include "shlwapi.h"
> +#include "shellapi.h"
> #include "winsvc.h"
> #include "wine/test.h"
>
> #define EXPECT_HR(hr,hr_exp) \
> ok(hr == hr_exp, "got 0x%08x, expected 0x%08x\n", hr, hr_exp)
>
> -static const WCHAR winetestW[] = {'w','i','n','e','t','e','s','t',0};
> +static const WCHAR winetestW[] = {'w','i','n','e','t','e','s','t',0,0};
>
> static HRESULT (WINAPI *pSHGetFolderPathW)(HWND, int, HANDLE, DWORD, LPWSTR);
> static HRESULT (WINAPI *pSHGetNameFromIDList)(PCIDLIST_ABSOLUTE,SIGDN,PWSTR*);
> @@ -310,19 +311,46 @@ static void test_namespace(void)
>
> static void test_items(void)
> {
> - WCHAR wstr[MAX_PATH], orig_dir[MAX_PATH];
> + static const struct
> + {
> + char name[32];
> + enum
> + {
> + DIRECTORY,
> + SHORTCUT,
> + EMPTY,
> + RANDOM,
> + }
> + type;
> + }
> + file_defs[] =
> + {
> + { "00-Myfolder", DIRECTORY },
> + { "01-empty.lnk", EMPTY },
Not really an issue, but is it intentional that the empty file has a .lnk extension?
> + { "02-shortcut.lnk", SHORTCUT },
> + { "03-empty.bin", EMPTY },
> + { "04-random.bin", RANDOM },
> + };
> + WCHAR wstr[MAX_PATH], wstr2[MAX_PATH], orig_dir[MAX_PATH];
> HRESULT r;
> IShellDispatch *sd = NULL;
> Folder *folder = NULL;
> FolderItems *items = NULL;
> FolderItems2 *items2 = NULL;
> FolderItems3 *items3 = NULL;
> - FolderItem *item = (FolderItem*)0xdeadbeef;
> + FolderItem *item = (FolderItem*)0xdeadbeef, *item2 = (FolderItem*)0xdeadbeef;
> IDispatch *disp = NULL;
> IUnknown *unk = NULL;
> FolderItemVerbs *verbs = (FolderItemVerbs*)0xdeadbeef;
> VARIANT var;
> LONG lcount = -1;
> + HANDLE file;
> + DWORD dwcount;
> + IPersistFile *shortcut = NULL;
> + BSTR bstr;
> + SHFILEOPSTRUCTW del;
> + char buf[512];
> + int i, j;
>
> r = CoCreateInstance(&CLSID_Shell, NULL, CLSCTX_INPROC_SERVER, &IID_IShellDispatch, (void**)&sd);
> ok(SUCCEEDED(r), "CoCreateInstance failed: %08x\n", r);
> @@ -345,13 +373,6 @@ static void test_items(void)
> r = Folder_Items(folder, &items);
> ok(r == S_OK, "Folder::Items failed: %08x\n", r);
> ok(!!items, "items is null\n");
> - r = FolderItems_QueryInterface(items, &IID_FolderItems2, (void**)&items2);
> - ok(r == S_OK || broken(E_NOINTERFACE) /* xp and later */, "FolderItems::QueryInterface failed: %08x\n", r);
> - ok(!!items2 || broken(!items2) /* xp and later */, "items2 is null\n");
> - r = FolderItems_QueryInterface(items, &IID_FolderItems3, (void**)&items3);
> - ok(r == S_OK, "FolderItems::QueryInterface failed: %08x\n", r);
> - ok(!!items3, "items3 is null\n");
> - Folder_Release(folder);
>
> if (0) /* crashes on all versions of Windows */
> r = FolderItems_get_Count(items, NULL);
> @@ -373,6 +394,179 @@ todo_wine
> ok(r == S_FALSE, "expected S_FALSE, got %08x\n", r);
> ok(!item, "item is not null\n");
>
> + for (i = 0; i < sizeof(file_defs)/sizeof(file_defs[0]); i++)
> + {
> + switch (file_defs[i].type)
> + {
> + case DIRECTORY:
> + CreateDirectoryA(file_defs[i].name, NULL);
> + PathCombineA(buf, file_defs[i].name, "foo.txt");
> + CloseHandle(CreateFileA(buf, 0, 0, NULL, CREATE_ALWAYS, FILE_ATTRIBUTE_NORMAL, NULL));
> + break;
> +
> + case SHORTCUT:
> + CoCreateInstance(&CLSID_ShellLink, NULL, CLSCTX_INPROC_SERVER, &IID_IPersistFile, (void**)&shortcut);
> + MultiByteToWideChar(CP_ACP, 0, file_defs[i].name, -1, wstr, sizeof(wstr)/sizeof(WCHAR));
> + IPersistFile_Save(shortcut, wstr, FALSE);
> + IPersistFile_Release(shortcut);
> + break;
> +
> + case EMPTY:
> + CloseHandle(CreateFileA(file_defs[i].name, 0, 0, NULL, CREATE_ALWAYS, FILE_ATTRIBUTE_NORMAL, NULL));
> + break;
> +
> + case RANDOM:
> + file = CreateFileA(file_defs[i].name, GENERIC_WRITE, 0, NULL,
> + CREATE_ALWAYS, FILE_ATTRIBUTE_NORMAL, NULL);
> + for (j = 0; j < sizeof(buf); j++)
> + buf[j] = rand() % 256;
> + WriteFile(file, buf, sizeof(buf), &dwcount, NULL);
> + CloseHandle(file);
> + break;
I think a few checks would not hurt in the code above.
> + }
> + }
> +
> + lcount = -1;
> + r = FolderItems_get_Count(items, &lcount);
> +todo_wine
> + ok(r == S_OK, "FolderItems::get_Count failed: %08x\n", r);
> +todo_wine
> + ok(!lcount, "expected 0 files, got %d\n", lcount);
> + FolderItems_Release(items);
> +
> + items = NULL;
> + r = Folder_Items(folder, &items);
> + ok(r == S_OK, "Folder::Items failed: %08x\n", r);
> + ok(!!items, "items is null\n");
> + r = FolderItems_QueryInterface(items, &IID_FolderItems2, (void**)&items2);
> + ok(r == S_OK || broken(E_NOINTERFACE) /* xp and later */, "FolderItems::QueryInterface failed: %08x\n", r);
> + if (r == S_OK) ok(!!items2, "items2 is null\n");
> + r = FolderItems_QueryInterface(items, &IID_FolderItems3, (void**)&items3);
> + ok(r == S_OK, "FolderItems::QueryInterface failed: %08x\n", r);
> + ok(!!items3, "items3 is null\n");
Isn't it necessary to release those items{2,3} interfaces?
> + Folder_Release(folder);
> +
> + lcount = -1;
> + r = FolderItems_get_Count(items, &lcount);
> +todo_wine
> + ok(r == S_OK, "FolderItems::get_Count failed: %08x\n", r);
> +todo_wine
> + ok(lcount == sizeof(file_defs)/sizeof(file_defs[0]),
> + "expected %d files, got %d\n", sizeof(file_defs)/sizeof(file_defs[0]), lcount);
> +
> + VariantInit(&var);
> + for (i = -10; i < 0x10000; i++)
> + {
> + item = NULL;
> + V_VT(&var) = i;
Is such a bruteforce approach also used in other places of Wine? I personally don't
think it makes much sense. It causes a huge amount of FIXMEs although there are just
a few supported types. For debugging purposes something like this is probably fine,
but it might be preferred to test only "reasonable" stuff. Please also note that that
VARTYPE is unsigned short, so negative numbers do not make sense. Very high numbers
correspond to vector/array/... which also don't make sense, especially when they are
not properly initialized.
> + r = FolderItems_Item(items, var, &item);
> + if (i == VT_I2 || i == VT_I4 || i == VT_ERROR)
> + {
> +todo_wine
> + ok(r == S_OK, "type %d: expected S_OK, got %08x\n", i, r);
> +todo_wine
> + ok(!!item, "item is null\n");
> + }
> + else if (i == VT_BSTR)
> + {
> +todo_wine
> + ok(r == S_FALSE, "type %d: expected S_FALSE, got %08x\n", i, r);
> + ok(!item, "item is not null\n");
> + }
> + else
> + {
> + ok(r == E_NOTIMPL, "type %d: expected E_NOTIMPL, got %08x\n", i, r);
> + ok(!item, "item is not null\n");
> + }
> + if (item) FolderItem_Release(item);
> + }
> +
> + for (i = -10; i < 10; i++)
> + {
Could you explain why this test is repeated 20 times? It does not look like you
actually use the counter variable anywhere.
> + item = NULL;
> + V_VT(&var) = VT_ERROR;
> + V_ERROR(&var) = 0;
> + r = FolderItems_Item(items, var, &item);
> +todo_wine
> + ok(r == S_OK, "expected S_OK, got %08x\n", r);
> +todo_wine
> + ok(!!item, "item is null\n");
> + if (!item) continue;
> +
> + bstr = NULL;
> + r = FolderItem_get_Path(item, &bstr);
> + ok(r == S_OK, "FolderItem::get_Path failed: %08x\n", r);
> + ok(!!bstr, "bstr is null\n");
> + GetCurrentDirectoryW(MAX_PATH, wstr);
> + GetLongPathNameW(wstr, wstr2, MAX_PATH);
> + ok(!lstrcmpW(bstr, wstr2),
> + "expected %s, got %s\n", wine_dbgstr_w(wstr2), wine_dbgstr_w(bstr));
> +
> + SysFreeString(bstr);
> + FolderItem_Release(item);
> + }
> +
> + item = NULL;
> + V_VT(&var) = VT_I4;
> + V_I4(&var) = -1;
> + r = FolderItems_Item(items, var, &item);
> +todo_wine
> + ok(r == S_FALSE, "expected S_FALSE, got %08x\n", r);
> + ok(!item, "item is not null\n");
Not sure if its critical, but the test isn't really convincing when you already
initialize item to the correct value. It might be better to set it to something else.
The same also applies to similar code below.
> +
> + for (i = 0; i < sizeof(file_defs)/sizeof(file_defs[0]); i++)
> + {
> + for (j = 0; j < 2; j++)
> + {
> + item = NULL;
It might be better to do the initialization directly in front of the call.
> + MultiByteToWideChar(CP_ACP, 0, file_defs[i].name, -1, wstr, MAX_PATH);
> +
> + if (j == 0)
> + {
> + V_VT(&var) = VT_I4;
> + V_I4(&var) = i;
> + }
> + else
> + {
> + V_VT(&var) = VT_BSTR;
> + V_BSTR(&var) = SysAllocString(wstr);
> + }
> +
> + r = FolderItems_Item(items, var, &item);
> +todo_wine
> + ok(r == S_OK, "file_defs[%d], %d: FolderItems::Item failed: %08x\n", i, j, r);
> +todo_wine
> + ok(!!item, "file_defs[%d], %d: item is null\n", i, j);
> + if (!item) continue;
> +
You probably should also initialize item2 here.
> + r = FolderItems_Item(items, var, &item2);
> + ok(r == S_OK, "file_defs[%d], %d: FolderItems::Item failed: %08x\n", i, j, r);
> + ok(item2 != item, "file_defs[%d], %d: item and item2 are the same\n", i, j);
> + FolderItem_Release(item2);
> + if (V_VT(&var) == VT_BSTR) SysFreeString(V_BSTR(&var));
> +
> + bstr = NULL;
> + r = FolderItem_get_Path(item, &bstr);
> + ok(r == S_OK, "file_defs[%d], %d: FolderItem::get_Path failed: %08x\n", i, j, r);
> + ok(!!bstr, "file_defs[%d], %d: bstr is null\n", i, j);
> + GetFullPathNameW(wstr, MAX_PATH, wstr2, NULL);
> + GetLongPathNameW(wstr2, wstr, MAX_PATH);
> + ok(!lstrcmpW(bstr, wstr),
> + "file_defs[%d], %d: expected %s, got %s\n", i, j, wine_dbgstr_w(wstr), wine_dbgstr_w(bstr));
> + SysFreeString(bstr);
> +
> + FolderItem_Release(item);
> + }
> + }
> +
> + V_VT(&var) = VT_I4;
> + V_I4(&var) = sizeof(file_defs)/sizeof(file_defs[0]);
> + item = NULL;
> + r = FolderItems_Item(items, var, &item);
> +todo_wine
> + ok(r == S_FALSE, "expected S_FALSE, got %08x\n", r);
> + ok(!item, "item is not null\n");
> +
> if (0) /* crashes on xp */
> {
> r = FolderItems_get_Application(items, NULL);
> @@ -430,7 +624,11 @@ todo_wine
>
> GetTempPathW(MAX_PATH, wstr);
> SetCurrentDirectoryW(wstr);
> - RemoveDirectoryW(winetestW);
> + memset(&del, 0, sizeof(del));
> + del.wFunc = FO_DELETE;
> + del.pFrom = winetestW;
> + del.fFlags = FOF_NO_UI;
> + SHFileOperationW(&del);
> SetCurrentDirectoryW(orig_dir);
>
> FolderItems_Release(items);
>
