[PATCH v2 2/3] windows.globalization: Implement IGlobalizationPreferencesStatics::Languages.

[Rémi Bernon]

On 3/25/21 10:46 PM, Jacek Caban wrote:
> Hi Rémi,
> 
> It looks mostly good, but error handling nuances could be improved.
> 
> On 3/25/21 9:17 AM, Rémi Bernon wrote:
>> +static HRESULT STDMETHODCALLTYPE 
>> hstring_vector_GetMany(IVectorView_HSTRING *iface,
>> +        ULONG start_index, ULONG items_size, HSTRING *items, UINT 
>> *count)
>> +{
>> +    struct hstring_vector *impl = impl_from_IVectorView_HSTRING(iface);
>> +    HRESULT hr;
>> +    ULONG i;
>> +
>> +    TRACE("iface %p, start_index %#x, items %p, count %p.\n", iface, 
>> start_index, items, count);
>> +
>> +    memset(items, 0, items_size * sizeof(HSTRING *));
>> +
>> +    for (i = start_index; i < impl->count && i < start_index + 
>> items_size; ++i)
>> +        if (FAILED(hr = WindowsDuplicateString(impl->values[i], items 
>> + i - start_index)))
>> +            return hr;
> 
> This leaks previously allocated strings in error case.
> 

Sure, then in this particular case the largest vector has one element, 
so it's not actually leaking anything. Also, the elements have not been 
created with WindowsCreateStringReference, so WindowsDuplicateString 
will only increments the string refcount count and is never going to fail.

>> +    WindowsCreateString(locale, wcslen(locale), &hstring);
> 
> This may fail.
> 

Here the only possible error is an allocation error. I honestly expect 
that things are going to be pretty bad very soon if we fail to allocate 
a few bytes. But yes, the status should be returned.

Still, I'll resend for the sake of correctness.

Cheers,
-- 
Rémi Bernon <rbernon at codeweavers.com>