Re: [5/7] windowscodecs: Add initial implementation of IWICMetadataQueryReader::GetMetadataByName.
+ 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?
Vincent Povirk <madewokherd(a)gmail.com> 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. -- Dmitry.
participants (2)
-
Dmitry Timoshkov -
Vincent Povirk