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

[Nikolay Sivov]

On 08.02.2016 3:07, Theodore Dubois wrote:
> > On Feb 7, 2016, at 4:25 AM, Nikolay Sivov  wrote:
> >
> > Why do you need this?
>
> So I can assert in Init.

That's not necessary.
> > >         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.
>
> I’ll fix that in the next version.
> > 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.
>
> In that case, hkeyProgID would refer to HKCR\exefile, and then I return the
> default value from that key which is “Application”. It used to start with hkeySource 
> (would point to HKCR.exe) and do some digging, but my code does that

work in Init.
> -
>    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;
> -
>    }

What happens if default value is not set?
> -
>    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);
> -
>    }

Now Init() will fail on missing progid subkey, and it didn't before.
Also this introduces leaks.
> > 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.
>
> Yes, more tests is always good. I’ll try and figure out how to test things on Windows and add some more tests in the next version.
>
> ~Theodore

[v2] shell32: fix two bugs in IQueryAssocations

[Theodore Dubois]

On Feb 7, 2016, at 4:38 PM, Nikolay Sivov  wrote:
> On 08.02.2016 3:07, Theodore Dubois wrote:
> > > On Feb 7, 2016, at 4:25 AM, Nikolay Sivov  wrote:
> > >
> > > Why do you need this?
> >
> > So I can assert in Init.
>
> That's not necessary.

If you don’t like it, I’ll take it out in the next version of the patch.
> > > >        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.
> >
> > I’ll fix that in the next version.
> > > 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.
> >
> > In that case, hkeyProgID would refer to HKCR\exefile, and then I return the
> > default value from that key which is “Application”. It used to start with hkeySource 
> > (would point to HKCR.exe) and do some digging, but my code does that
>
> work in Init.
> > -
> >    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;
> > -
> >    }
>
> What happens if default value is not set?

It would return HRESULT_FROM_WIN32(ERROR_FILE_NOT_FOUND). Windows reverts to all the defaults, such as capitalizing the file extension and adding “File” when you ask for the doc name. So, probably should not fail here, and the doc name code should be fixed so it does that when hkeyProgID is null.
> > -
> >    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);
> > -
> >    }
>
> Now Init() will fail on missing progid subkey, and it didn't before.
> Also this introduces leaks.

Again, instead of failing, I really should just revert to default values in GetString. And yes, I forgot to free progId. I’ll fix that.

~Theodore