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

[Nikolay Sivov]

On 07.02.2016 1:09, 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 <tblodt(a)icloud.com>
> ---
>  dlls/shell32/assoc.c       | 115 +++++++++++++++++++++++----------------------
>  dlls/shell32/tests/assoc.c |  12 ++++-
>  2 files changed, 70 insertions(+), 57 deletions(-)
>
> diff --git a/dlls/shell32/assoc.c b/dlls/shell32/assoc.c
> index 90d00a8..49af3c3 100644
> --- a/dlls/shell32/assoc.c
> +++ b/dlls/shell32/assoc.c
> @@ -18,6 +18,7 @@
>   * Foundation, Inc., 51 Franklin St, Fifth Floor, Boston, MA 02110-1301, USA
>   */
>  #include <stdarg.h>
> +#include <assert.h>

Why do you need this?
> This->hkeyProgID = This->hkeySource;

I know it's not something you introduced, but it's dirty and should be
fixed, because it leads to double free on a same handle.
> +            return S_OK;
> +        }
> +
> +        /* if it's not a progid, it's a file extension or clsid */
> +        if (*pszAssoc == '.')
> +        {
> +            /* for a file extension, the progid is the default value */
> +            hr = ASSOC_GetValue(This->hkeySource, NULL, (void**)&progId, NULL);
> +            if (FAILED(hr))
> +                return hr;
> +        }
> +        else /* if (*pszAssoc == '{') */
> +        {
> +            assert(*pszAssoc == '{');
> +
> +            HKEY progIdKey;
> +            /* for a clsid, the progid is the default value of the ProgID subkey */
> +            ret = RegOpenKeyExW(This->hkeySource,
> +                                szProgID,
> +                                0,
> +                                KEY_READ,
> +                                &progIdKey);
> +            if (ret != ERROR_SUCCESS)
> +                return HRESULT_FROM_WIN32(ret);
> +            hr = ASSOC_GetValue(progIdKey, NULL, (void**)&progId, NULL);
> +            if (FAILED(hr))
> +                return hr;
> +            RegCloseKey(progIdKey);
> +        }
> +        
> +        /* open the actual progid key, the one with the shell subkey */
> +        ret = RegOpenKeyExW(HKEY_CLASSES_ROOT,
> +                            progId,
> +                            0,
> +                            KEY_READ,
> +                            &This->hkeyProgID);
> +        if (ret != ERROR_SUCCESS)
> +            return HRESULT_FROM_WIN32(ret);
>          return S_OK;
>      }
>      else if (hkeyProgid != NULL)
> @@ -507,33 +544,12 @@ static HRESULT WINAPI IQueryAssociations_fnGetString(
>
> case ASSOCSTR_FRIENDLYDOCNAME:
>      {
> -      WCHAR *pszFileType;
> -      DWORD ret;
> -      DWORD size;
> +      WCHAR *docName;
>
> -      hr = ASSOC_GetValue(This->hkeySource, NULL, (void**)&pszFileType, NULL);
> +      hr = ASSOC_GetValue(This->hkeyProgID, NULL, (void**)&docName, NULL);
>        if (FAILED(hr))
>          return hr;
> -      size = 0;
> -      ret = RegGetValueW(HKEY_CLASSES_ROOT, pszFileType, NULL, RRF_RT_REG_SZ, NULL, NULL, &size);
> -      if (ret == ERROR_SUCCESS)
> -      {
> -        WCHAR *docName = HeapAlloc(GetProcessHeap(), 0, size);
> -        if (docName)
> -        {
> -          ret = RegGetValueW(HKEY_CLASSES_ROOT, pszFileType, NULL, RRF_RT_REG_SZ, NULL, docName, &size);
> -          if (ret == ERROR_SUCCESS)
> -            hr = ASSOC_ReturnString(flags, pszOut, pcchOut, docName, strlenW(docName) + 1);
> -          else
> -            hr = HRESULT_FROM_WIN32(ret);
> -          HeapFree(GetProcessHeap(), 0, docName);
> -        }
> -        else
> -          hr = E_OUTOFMEMORY;
> -      }
> -      else
> -        hr = HRESULT_FROM_WIN32(ret);
> -      HeapFree(GetProcessHeap(), 0, pszFileType);
> +      hr = ASSOC_ReturnString(flags, pszOut, pcchOut, docName, strlenW(docName) + 1);
>        return hr;
>      }

Basically you remove everything, and tests don't show that you should do
that. For example looking at Win7 registry I see HKCR\.exe with default
value of "exefile", then default value for HKCR\exefile is Application
(and also there is a FriendlyTypeName key that's most likely localized).
Current code will return "Application" and that seems like a better
candidate for a friendly name.

A convincing test should create a custom extension key, and test all
possible combinations of empty default values, missing keys with default
value name etc.
> @@ -623,41 +639,28 @@ get_friendly_name_fail:
>      case ASSOCSTR_DEFAULTICON:
>      {
>        static const WCHAR DefaultIconW[] = {'D','e','f','a','u','l','t','I','c','o','n',0};
> -      WCHAR *pszFileType;
>        DWORD ret;
>        DWORD size;
> -      HKEY hkeyFile;
>
> -      hr = ASSOC_GetValue(This->hkeySource, NULL, (void**)&pszFileType, NULL);
> -      if (FAILED(hr))
> -        return hr;
> -      ret = RegOpenKeyExW(HKEY_CLASSES_ROOT, pszFileType, 0, KEY_READ, &hkeyFile);
> +      size = 0;
> +      ret = RegGetValueW(This->hkeyProgID, DefaultIconW, NULL, RRF_RT_REG_SZ, NULL, NULL, &size);
>        if (ret == ERROR_SUCCESS)
>        {
> -        size = 0;
> -        ret = RegGetValueW(hkeyFile, DefaultIconW, NULL, RRF_RT_REG_SZ, NULL, NULL, &size);
> -        if (ret == ERROR_SUCCESS)
> +        WCHAR *icon = HeapAlloc(GetProcessHeap(), 0, size);
> +        if (icon)
>          {
> -          WCHAR *icon = HeapAlloc(GetProcessHeap(), 0, size);
> -          if (icon)
> -          {
> -            ret = RegGetValueW(hkeyFile, DefaultIconW, NULL, RRF_RT_REG_SZ, NULL, icon, &size);
> -            if (ret == ERROR_SUCCESS)
> -              hr = ASSOC_ReturnString(flags, pszOut, pcchOut, icon, strlenW(icon) + 1);
> -            else
> -              hr = HRESULT_FROM_WIN32(ret);
> -            HeapFree(GetProcessHeap(), 0, icon);
> -          }
> +          ret = RegGetValueW(This->hkeyProgID, DefaultIconW, NULL, RRF_RT_REG_SZ, NULL, icon, &size);
> +          if (ret == ERROR_SUCCESS)
> +            hr = ASSOC_ReturnString(flags, pszOut, pcchOut, icon, strlenW(icon) + 1);
>            else
> -            hr = E_OUTOFMEMORY;
> +            hr = HRESULT_FROM_WIN32(ret);
> +          HeapFree(GetProcessHeap(), 0, icon);
>          }
>          else
> -          hr = HRESULT_FROM_WIN32(ret);
> -        RegCloseKey(hkeyFile);
> +          hr = E_OUTOFMEMORY;
>        }
>        else
>          hr = HRESULT_FROM_WIN32(ret);
> -      HeapFree(GetProcessHeap(), 0, pszFileType);
>        return hr;
>      }

Same.