2016-08-18 2:58 GMT-06:00 Sebastian Lackner sebastian@fds-team.de:
On 18.08.2016 06:37, Alex Henrie wrote:
Cc: Sebastian Lackner sebastian@fds-team.de
v7 fixes the use-after-free of V_BSTR(&var).
Signed-off-by: Alex Henrie alexhenrie24@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