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

[Sven Baars]

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
> >
> > 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.

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?

Best,
Sven