Re: [5/7] windowscodecs: Add initial implementation of IWICMetadataQueryReader::GetMetadataByName.

[Vincent Povirk]

> -
>    if (vt == VT_CLSID)
> -
>    {
> -
>        id->u.puuid = CoTaskMemAlloc(sizeof(GUID));
> -
>        if (!id->u.puuid) return E_OUTOFMEMORY;
> -
> -
>        hr = UuidFromStringW(next_token.u.bstrVal, id->u.puuid);
> -
>    }
> -
>    else
> -
>        hr = PropVariantChangeType(id, &next_token, 0, vt);
> -
>    PropVariantClear(&next_token);
> -
>    if (hr != S_OK)
> -
>    {
> -
>        PropVariantClear(id);
> -
>        PropVariantClear(schema);
> -
>        return hr;
> -
>    }
> -
> -
>    id->vt = vt;

Assigning the vt at the end of this is a bit strange. I don't know any
situation here where PropVariantChangeType will succeed without giving
you the requested vt, and if it did, changing it would be unsafe.

I can see you need it in the VT_CLSID case, but then it would make
more sense to set it there. In fact, this version will leak memory if
UuidFromStringW fails.

+        if (!elem.len) break;

When can this happen?

+            new_value.vt = VT_UNKNOWN;
+            hr = MetadataQueryReader_CreateInstance(This->block,
root, (IWICMetadataQueryReader **)&new_value.u.punkVal);

I think you need to clear new_value first here?

[5/7] windowscodecs: Add initial implementation of IWICMetadataQueryReader::GetMetadataByName.

[Dmitry Timoshkov]

Vincent Povirk  wrote:
> > -
> >    if (vt == VT_CLSID)
> > -
> >    {
> > -
> >        id->u.puuid = CoTaskMemAlloc(sizeof(GUID));
> > -
> >        if (!id->u.puuid) return E_OUTOFMEMORY;
> > -
> > -
> >        hr = UuidFromStringW(next_token.u.bstrVal, id->u.puuid);
> > -
> >    }
> > -
> >    else
> > -
> >        hr = PropVariantChangeType(id, &next_token, 0, vt);
> > -
> >    PropVariantClear(&next_token);
> > -
> >    if (hr != S_OK)
> > -
> >    {
> > -
> >        PropVariantClear(id);
> > -
> >        PropVariantClear(schema);
> > -
> >        return hr;
> > -
> >    }
> > -
> > -
> >    id->vt = vt;
>
> Assigning the vt at the end of this is a bit strange. I don't know any
> situation here where PropVariantChangeType will succeed without giving
> you the requested vt, and if it did, changing it would be unsafe.
>
> I can see you need it in the VT_CLSID case, but then it would make
> more sense to set it there. In fact, this version will leak memory if
> UuidFromStringW fails.

Thanks for spotting this bug.
> -
>    if (!elem.len) break;
>
> When can this happen?

This could happen when the query ends with '/'. That's something that
I noticed while playing with my test app, I have not included a test
case for this in the test suite though.
> -
>        new_value.vt = VT_UNKNOWN;
> -
>        hr = MetadataQueryReader_CreateInstance(This->block,
>
> root, (IWICMetadataQueryReader **)&new_value.u.punkVal);
>
> I think you need to clear new_value first here?

Yep, thanks.