Re: [PATCH] shell32: fix two bugs in IQueryAssocations

[Nikolay Sivov]

On 14.02.2016 2:19, Theodore Dubois wrote:
> Basically, returns the correct value for
> ASSOCSTR_FRIENDLYDOCNAME and ASSOCSTR_DEFAULTICON
> when passed a ProgID instead of a file extension.
>
> Also adds some tests to make sure it works.
> Signed-off-by: Theodore Dubois
>
> dlls/shell32/assoc.c       | 146 +++++++++++++++++++++++++--------------------
>  dlls/shell32/tests/assoc.c |  71 +++++++++++++++++++++-
>  2 files changed, 151 insertions(+), 66 deletions(-)
>
> diff --git a/dlls/shell32/assoc.c b/dlls/shell32/assoc.c
> index 90d00a8..2667ffe 100644
> --- a/dlls/shell32/assoc.c
> +++ b/dlls/shell32/assoc.c
> @@ -63,6 +63,7 @@ typedef struct
>    LONG ref;
>    HKEY hkeySource;
>    HKEY hkeyProgID;
> - LPWSTR pszAssocName;
>
> } IQueryAssociationsImpl;

Why do you need this? It's not used anywhere as far as I can tell.
> typedef struct
> @@ -88,6 +89,8 @@ static inline struct enumassochandlers *impl_from_IEnumAssocHandlers(IEnumAssocH
>    return CONTAINING_RECORD(iface, struct enumassochandlers, IEnumAssocHandlers_iface);
>  }
>
> +static HRESULT ASSOC_GetValue(HKEY hkey, const WCHAR *name, void **data, DWORD *data_size);
> -
>
> /**************************************************************************
> - IQueryAssociations_QueryInterface
> -
>
> @@ -152,6 +155,7 @@ static ULONG WINAPI IQueryAssociations_fnRelease(IQueryAssociations *iface)
>      TRACE("Destroying IQueryAssociations (%p)\n", This);
>      RegCloseKey(This->hkeySource);
>      RegCloseKey(This->hkeyProgID);
> - HeapFree(GetProcessHeap(), 0, (LPVOID)This->pszAssocName);
> SHFree(This);
> }

No need for a cast.
> @@ -186,20 +190,30 @@ static HRESULT WINAPI IQueryAssociations_fnInit(
>      LONG ret;
>  TRACE("(%p)->(%d,%s,%p,%p)\n", iface,
> -
>                                cfFlags,
> -
>                                debugstr_w(pszAssoc),
> -
>                                hkeyProgid,
> -
>                                hWnd);
> -
>                               cfFlags,
> -
>                               debugstr_w(pszAssoc),
> -
>                               hkeyProgid,
> -
>                               hWnd);

No need for that either.
> -
> -
>    This->pszAssocName = HeapAlloc(GetProcessHeap(), 0, (strlenW(pszAssoc) + 10001) * sizeof(WCHAR));

Seriously?
> - *data = HeapAlloc(GetProcessHeap(), 0, size);
> - *data = HeapAlloc(GetProcessHeap(), 0, size + 100);

What is that?
> if (!*data)
>      return E_OUTOFMEMORY;
>    ret = RegQueryValueExW(hkey, name, 0, NULL, (LPBYTE)*data, &size);
> @@ -507,33 +550,16 @@ static HRESULT WINAPI IQueryAssociations_fnGetString(
>  case ASSOCSTR_FRIENDLYDOCNAME:
>  {
> -
>  WCHAR *pszFileType;
> -
>  DWORD ret;
> -
>  DWORD size;
> -
>  WCHAR *docName;
> -
> -
>  if (This->hkeyProgID == NULL) {
> -
> -
>  }

And that?
> diff --git a/dlls/shell32/tests/assoc.c b/dlls/shell32/tests/assoc.c
> index 8aa2535..1ba4261 100644
> --- a/dlls/shell32/tests/assoc.c
> +++ b/dlls/shell32/tests/assoc.c
> @@ -91,6 +91,8 @@ struct assoc_getstring_test
>  };
>
> static const WCHAR httpW[] = {'h','t','t','p',0};
> +static const WCHAR dot_urlW[] = {'.','u','r','l',0};
> +static const WCHAR InternetShortcutW[] = {'I','n','t','e','r','n','e','t','S','h','o','r','t','c','u','t',0};
>  static const WCHAR badW[] = {'b','a','d','b','a','d',0};
>
> static struct assoc_getstring_test getstring_tests[] =
> @@ -135,7 +137,7 @@ static void test_IQueryAssociations_GetString(void)
>          else
>          {
>              ok(hr == ptr->hr, "%d: GetString failed, 0x%08x\n", i, hr);
> -
>        ok(len > ptr->len, "%d: got needed length %d\n", i, len);
> -
>        ok(len >= ptr->len, "%d: got needed length %d\n", i, len);
>    }

Why '>' was enough before?
> - getstring_test(0, NULL, test_progid_key, ASSOCSTR_DEFAULTICON, NULL, S_OK, test_iconW);
> -

If it's only called with 0 flags, there's no need in this argument.
> - //RegDeleteKeyExW(HKEY_CLASSES_ROOT, test_extensionW, KEY_WOW64_32KEY, 0);
> - //RegDeleteKeyExW(HKEY_CLASSES_ROOT, test_progidW, KEY_WOW64_32KEY, 0);
>
> +}

Leftovers?
> -
>
> static void test_IQueryAssociations_Init(void)
>  {
>      IQueryAssociations *assoc;
> @@ -229,6 +297,7 @@ START_TEST(assoc)
>      if (hr == S_OK)
>      {
>          test_IQueryAssociations_QueryInterface();
> -
>    test_IQueryAssociations_GetString_results();
>    test_IQueryAssociations_GetString();
>    test_IQueryAssociations_Init();

Please merge this with existing *_GetString().

[PATCH] shell32: fix two bugs in IQueryAssocations

[Theodore Dubois]

This is sort of a work in progress. I just wanted to see what Marvin had to say about it. Please ignore for now.

~Theodore
> On Feb 13, 2016, at 3:34 PM, Nikolay Sivov  wrote:
>
> On 14.02.2016 2:19, Theodore Dubois wrote:
> > Basically, returns the correct value for
> > ASSOCSTR_FRIENDLYDOCNAME and ASSOCSTR_DEFAULTICON
> > when passed a ProgID instead of a file extension.
> >
> > Also adds some tests to make sure it works.
> > Signed-off-by: Theodore Dubois
> >
> > dlls/shell32/assoc.c       | 146 +++++++++++++++++++++++++--------------------
> > dlls/shell32/tests/assoc.c |  71 +++++++++++++++++++++-
> > 2 files changed, 151 insertions(+), 66 deletions(-)
> >
> > diff --git a/dlls/shell32/assoc.c b/dlls/shell32/assoc.c
> > index 90d00a8..2667ffe 100644
> > --- a/dlls/shell32/assoc.c
> > +++ b/dlls/shell32/assoc.c
> > @@ -63,6 +63,7 @@ typedef struct
> >  LONG ref;
> >  HKEY hkeySource;
> >  HKEY hkeyProgID;
> > - LPWSTR pszAssocName;
> >
> > } IQueryAssociationsImpl;
>
> Why do you need this? It's not used anywhere as far as I can tell.
> > typedef struct
> > @@ -88,6 +89,8 @@ static inline struct enumassochandlers *impl_from_IEnumAssocHandlers(IEnumAssocH
> >  return CONTAINING_RECORD(iface, struct enumassochandlers, IEnumAssocHandlers_iface);
> > }
> >
> > +static HRESULT ASSOC_GetValue(HKEY hkey, const WCHAR *name, void **data, DWORD *data_size);
> > -
> >
> > /**************************************************************************
> > - IQueryAssociations_QueryInterface
> > -
> >
> > @@ -152,6 +155,7 @@ static ULONG WINAPI IQueryAssociations_fnRelease(IQueryAssociations *iface)
> >    TRACE("Destroying IQueryAssociations (%p)\n", This);
> >    RegCloseKey(This->hkeySource);
> >    RegCloseKey(This->hkeyProgID);
> > - HeapFree(GetProcessHeap(), 0, (LPVOID)This->pszAssocName);
> > SHFree(This);
> >
> > }
>
> No need for a cast.
> > @@ -186,20 +190,30 @@ static HRESULT WINAPI IQueryAssociations_fnInit(
> >    LONG ret;
> >
> > TRACE("(%p)->(%d,%s,%p,%p)\n", iface,
> > -
> >                                cfFlags,
> > -
> >                                debugstr_w(pszAssoc),
> > -
> >                                hkeyProgid,
> > -
> >                                hWnd);
> > -
> >                               cfFlags,
> > -
> >                               debugstr_w(pszAssoc),
> > -
> >                               hkeyProgid,
> > -
> >                               hWnd);
>
> No need for that either.
> > -
> > -
> >    This->pszAssocName = HeapAlloc(GetProcessHeap(), 0, (strlenW(pszAssoc) + 10001) * sizeof(WCHAR));
>
> Seriously?
> > - *data = HeapAlloc(GetProcessHeap(), 0, size);
> > - *data = HeapAlloc(GetProcessHeap(), 0, size + 100);
>
> What is that?
> > if (!*data)
> >    return E_OUTOFMEMORY;
> >  ret = RegQueryValueExW(hkey, name, 0, NULL, (LPBYTE)*data, &size);
> > @@ -507,33 +550,16 @@ static HRESULT WINAPI IQueryAssociations_fnGetString(
> >
> > case ASSOCSTR_FRIENDLYDOCNAME:
> >    {
> > -
> >  WCHAR *pszFileType;
> > -
> >  DWORD ret;
> > -
> >  DWORD size;
> > -
> >  WCHAR *docName;
> > -
> > -
> >  if (This->hkeyProgID == NULL) {
> > -
> > -
> >  }
>
> And that?
> > diff --git a/dlls/shell32/tests/assoc.c b/dlls/shell32/tests/assoc.c
> > index 8aa2535..1ba4261 100644
> > --- a/dlls/shell32/tests/assoc.c
> > +++ b/dlls/shell32/tests/assoc.c
> > @@ -91,6 +91,8 @@ struct assoc_getstring_test
> > };
> >
> > static const WCHAR httpW[] = {'h','t','t','p',0};
> > +static const WCHAR dot_urlW[] = {'.','u','r','l',0};
> > +static const WCHAR InternetShortcutW[] = {'I','n','t','e','r','n','e','t','S','h','o','r','t','c','u','t',0};
> > static const WCHAR badW[] = {'b','a','d','b','a','d',0};
> >
> > static struct assoc_getstring_test getstring_tests[] =
> > @@ -135,7 +137,7 @@ static void test_IQueryAssociations_GetString(void)
> >        else
> >        {
> >            ok(hr == ptr->hr, "%d: GetString failed, 0x%08x\n", i, hr);
> > -
> >        ok(len > ptr->len, "%d: got needed length %d\n", i, len);
> > -
> >        ok(len >= ptr->len, "%d: got needed length %d\n", i, len);
> >  }
>
> Why '>' was enough before?
> > - getstring_test(0, NULL, test_progid_key, ASSOCSTR_DEFAULTICON, NULL, S_OK, test_iconW);
> > -
>
> If it's only called with 0 flags, there's no need in this argument.
> > - //RegDeleteKeyExW(HKEY_CLASSES_ROOT, test_extensionW, KEY_WOW64_32KEY, 0);
> > - //RegDeleteKeyExW(HKEY_CLASSES_ROOT, test_progidW, KEY_WOW64_32KEY, 0);
> >
> > +}
>
> Leftovers?
> > -
> >
> > static void test_IQueryAssociations_Init(void)
> > {
> >    IQueryAssociations *assoc;
> > @@ -229,6 +297,7 @@ START_TEST(assoc)
> >    if (hr == S_OK)
> >    {
> >        test_IQueryAssociations_QueryInterface();
> > -
> >    test_IQueryAssociations_GetString_results();
> >  test_IQueryAssociations_GetString();
> >  test_IQueryAssociations_Init();
>
> Please merge this with existing *_GetString().