Re: [PATCH v7 1/3] shell32/tests: Add tests for FolderItems_Item and FolderItems_get_Count.

[Sebastian Lackner]

On 18.08.2016 06:37, Alex Henrie wrote:
> Cc: Sebastian Lackner
>
> v7 fixes the use-after-free of V_BSTR(&var).
> Signed-off-by: Alex Henrie
>
> 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);

[PATCH v7 1/3] shell32/tests: Add tests for FolderItems_Item and FolderItems_get_Count.

[Alex Henrie]

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