[PATCH 2/3] scrobj: Fix some leaks on error paths (Coverity).

[Jacek Caban]

On 21/10/2019 07:50, Sven Baars wrote:
> On 21-10-19 12:14, Jacek Caban wrote:
>> Hi Sven,
>>
>> On 19/10/2019 15:20, Sven Baars wrote:
>>> Signed-off-by: Sven Baars <sven.wine at gmail.com>
>>> ---
>>>    dlls/scrobj/scrobj.c | 7 ++++++-
>>>    1 file changed, 6 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/dlls/scrobj/scrobj.c b/dlls/scrobj/scrobj.c
>>> index ce5a3dedd1..8d23c65f55 100644
>>> --- a/dlls/scrobj/scrobj.c
>>> +++ b/dlls/scrobj/scrobj.c
>>> @@ -1491,6 +1491,7 @@ static HRESULT parse_scriptlet_public(struct
>>> scriptlet_factory *factory)
>>>                if (!wcsicmp(member_iter->name, member->name))
>>>                {
>>>                    FIXME("Duplicated member %s\n",
>>> debugstr_w(member->name));
>>> +                heap_free(member);
>>>                    return E_FAIL;
>>>                }
>>>            }
>>> @@ -1532,7 +1533,11 @@ static HRESULT parse_scriptlet_public(struct
>>> scriptlet_factory *factory)
>>>                    if (!(parameter = heap_alloc(sizeof(*parameter))))
>>> return E_OUTOFMEMORY;
>>>                      hres = read_xml_value(factory, &parameter->name);
>>> -                if (FAILED(hres)) return hres;
>>> +                if (FAILED(hres))
>>> +                {
>>> +                    heap_free(parameter);
>>> +                    return hres;
>>> +                }
>>>                    list_add_tail(&member->u.parameters,
>>> &parameter->entry);
>>>                    if (!empty && FAILED(hres =
>>> expect_end_element(factory))) return hres;
>>>                }
>>
>> You could just move list_add_tail() call to be done just after heap_alloc.
>>
>>
>> Thanks,
>>
>> Jacek
>>
> Hi Jacek,
>
> But then you have a "parameter", whose only member is "name", that does
> not have a name, since the name is set by read_xml_value, but that
> method failed, meaning the name has not been set. Therefore I thought it
> was better not to add it to the list.


In this case, we will fail to create the factory, so we will call its 
destructor (scriptlet_factory_Release), which should handle it just fine 
and free the parameter.


> I actually don't really understand what this code is supposed to do
> anyway. The "parameters" do never seem to be used anywhere. The only
> "scriptlet_member" members that appear to be used are "type" and "name"
> (and maybe "flags" for validation). Is this for a future implementation?


Yes, nothing I've seen used needed it so far. We have to parse the XML 
anyway and I guess that we may need it for GetTypeInfo() implementation 
at some point.


Thanks,

Jacek