[PATCH 1/3] scrobj/tests: Don't set site to NULL before it has been freed (Valgrind).

[Sven Baars]

Signed-off-by: Sven Baars 
---
 dlls/scrobj/tests/scrobj.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/dlls/scrobj/tests/scrobj.c b/dlls/scrobj/tests/scrobj.c
index 023aa30f92..861725c41a 100644
--- a/dlls/scrobj/tests/scrobj.c
+++ b/dlls/scrobj/tests/scrobj.c
@@ -634,7 +634,6 @@ static HRESULT WINAPI ClassFactory_CreateInstance(IClassFactory *iface, IUnknown
     ok(!outer, "outer = %p\n", outer);
     ok(IsEqualGUID(&IID_IUnknown, riid), "unexpected riid %s\n", wine_dbgstr_guid(riid));
     *ppv = &ActiveScript;
-    site = NULL;
     return S_OK;
 }

@@ -750,6 +749,7 @@ static void register_script_object(BOOL do_register, const WCHAR *file_name)
     HRESULT hres;

parse_flags = SCRIPTTEXT_ISPERSISTENT | SCRIPTTEXT_ISVISIBLE;
+    site = NULL;

SET_EXPECT(CreateInstance);
     SET_EXPECT(QI_IActiveScriptParse);

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

[Sven Baars]

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;
             }

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

[Jacek Caban]

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

[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

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

[PATCH 3/3] scrobj: Add a missing return value check (Coverity).

[Sven Baars]

Signed-off-by: Sven Baars 
---
 dlls/scrobj/scrobj.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/dlls/scrobj/scrobj.c b/dlls/scrobj/scrobj.c
index 8d23c65f55..aa6ed0c6b5 100644
--- a/dlls/scrobj/scrobj.c
+++ b/dlls/scrobj/scrobj.c
@@ -1699,6 +1699,8 @@ static HRESULT parse_scriptlet_file(struct scriptlet_factory *factory, const WCH
     hres = next_xml_node(factory, &node_type);
     if (hres == S_OK && node_type == XmlNodeType_XmlDeclaration)
         hres = next_xml_node(factory, &node_type);
+    if (FAILED(hres))
+        return hres;

if (node_type != XmlNodeType_Element || !is_xml_name(factory, L"component"))
     {

[PATCH 3/3] scrobj: Add a missing return value check (Coverity).

[Jacek Caban]

Signed-off-by: Jacek Caban

[Jacek Caban]

Hi Sven,

On 19/10/2019 15:20, Sven Baars wrote:
> @@ -750,6 +749,7 @@ static void register_script_object(BOOL do_register, const WCHAR *file_name)
>       HRESULT hres;
>   parse_flags = SCRIPTTEXT_ISPERSISTENT | SCRIPTTEXT_ISVISIBLE;
> - site = NULL;

Do we need to set it here? It seems to me that Close() should already 
take care of that.

Thanks,

Jacek

[Sven Baars]

On 21-10-19 12:17, Jacek Caban wrote:
> Hi Sven,
>
> On 19/10/2019 15:20, Sven Baars wrote:
> > @@ -750,6 +749,7 @@ static void register_script_object(BOOL
> > do_register, const WCHAR *file_name)
> >       HRESULT hres;
> >         parse_flags = SCRIPTTEXT_ISPERSISTENT | SCRIPTTEXT_ISVISIBLE;
> > +    site = NULL;
>
> Do we need to set it here? It seems to me that Close() should already
> take care of that.
>
> Thanks,
>
> Jacek

Hi Jacek,

I did that to initialize it to NULL before it gets used in the other
methods. Else it would be uninitialized.

Best,
Sven

[Jacek Caban]

On 21/10/2019 07:16, Sven Baars wrote:
> On 21-10-19 12:17, Jacek Caban wrote:
> > Hi Sven,
> >
> > On 19/10/2019 15:20, Sven Baars wrote:
> > > @@ -750,6 +749,7 @@ static void register_script_object(BOOL
> > > do_register, const WCHAR *file_name)
> > >        HRESULT hres;
> > >          parse_flags = SCRIPTTEXT_ISPERSISTENT | SCRIPTTEXT_ISVISIBLE;
> > > +    site = NULL;
> >
> > Do we need to set it here? It seems to me that Close() should already
> > take care of that.
> >
> > Thanks,
> >
> > Jacek
>
> Hi Jacek,
>
> I did that to initialize it to NULL before it gets used in the other
> methods. Else it would be uninitialized.

It's a static variable, so it will be initialized to NULL.

Jacek