[PATCH v7 1/3] shell32/tests: Add tests for FolderItems_Item and FolderItems_get_Count.
Alex Henrie
alexhenrie24 at gmail.com
Thu Aug 18 11:35:08 CDT 2016
2016-08-18 2:58 GMT-06:00 Sebastian Lackner <sebastian at fds-team.de>:
> On 18.08.2016 06:37, Alex Henrie wrote:
>> Cc: Sebastian Lackner <sebastian at fds-team.de>
>>
>> v7 fixes the use-after-free of V_BSTR(&var).
>>
>> Signed-off-by: Alex Henrie <alexhenrie24 at 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?
Yes. I wanted to test that an empty file with a .lnk extension is
treated the same as a real shortcut.
>> + { "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.
Okay. I'll add checks that the files were created and written successfully.
>> + }
>> + }
>> +
>> + 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?
They are already released at the end of the function.
>> + 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.
Yes, for example when we test the UTF-7 conversion of every Unicode
character. This code is how I found out that VT_ERROR has special
behavior.
> 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.
The FIXMEs are removed in the next patch. You're right that the
negative number tests are redundant; I'll remove them.
>> + 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.
There's a mistake here; it was supposed to set V_ERROR(&var) = i on
every iteration.
>> + 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.
Good point.
>> +
>> + 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.
Okay.
>> + 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.
If FolderItems_Item succeeds the first time, there is no reason for it
to fail the second time. But I will reinitialize item2 just in case.
>> + 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);
>>
>
Thanks for the review!
-Alex
More information about the wine-devel
mailing list