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
>
> 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.