[v1] spapr: Fix Coverity warning while validating nvdimm options

[PATCH] spapr: Fix Coverity warning while validating nvdimm options

Posted by Shivaprasad G Bhat 5 years, 8 months ago

Fixes Coverity issue,
      CID 1419883:  Error handling issues  (CHECKED_RETURN)
           Calling "qemu_uuid_parse" without checking return value

nvdimm_set_uuid() already verifies if the user provided uuid is valid or
not. So, need to check for the validity during pre-plug validation again.

As this a false positive in this case, assert if not valid to be safe.

Reported-by: Coverity (CID 1419883)
Signed-off-by: Shivaprasad G Bhat <sbhat@linux.ibm.com>
---
 hw/ppc/spapr_nvdimm.c |    2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/hw/ppc/spapr_nvdimm.c b/hw/ppc/spapr_nvdimm.c
index 74eeb8bb74..051727536e 100644
--- a/hw/ppc/spapr_nvdimm.c
+++ b/hw/ppc/spapr_nvdimm.c
@@ -44,7 +44,7 @@ void spapr_nvdimm_validate_opts(NVDIMMDevice *nvdimm, uint64_t size,
     }
 
     uuidstr = object_property_get_str(OBJECT(nvdimm), NVDIMM_UUID_PROP, NULL);
-    qemu_uuid_parse(uuidstr, &uuid);
+    g_assert(qemu_uuid_parse(uuidstr, &uuid) == 0);
     g_free(uuidstr);
 
     if (qemu_uuid_is_null(&uuid)) {

Re: [PATCH] spapr: Fix Coverity warning while validating nvdimm options

Posted by Philippe Mathieu-Daudé 5 years, 8 months ago

Le mer. 26 févr. 2020 13:11, Shivaprasad G Bhat <sbhat@linux.ibm.com> a
écrit :

> Fixes Coverity issue,
>       CID 1419883:  Error handling issues  (CHECKED_RETURN)
>            Calling "qemu_uuid_parse" without checking return value
>
> nvdimm_set_uuid() already verifies if the user provided uuid is valid or
> not. So, need to check for the validity during pre-plug validation again.
>
> As this a false positive in this case, assert if not valid to be safe.
>
> Reported-by: Coverity (CID 1419883)
> Signed-off-by: Shivaprasad G Bhat <sbhat@linux.ibm.com>
> ---
>  hw/ppc/spapr_nvdimm.c |    2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/hw/ppc/spapr_nvdimm.c b/hw/ppc/spapr_nvdimm.c
> index 74eeb8bb74..051727536e 100644
> --- a/hw/ppc/spapr_nvdimm.c
> +++ b/hw/ppc/spapr_nvdimm.c
> @@ -44,7 +44,7 @@ void spapr_nvdimm_validate_opts(NVDIMMDevice *nvdimm,
> uint64_t size,
>      }
>
>      uuidstr = object_property_get_str(OBJECT(nvdimm), NVDIMM_UUID_PROP,
> NULL);
> -    qemu_uuid_parse(uuidstr, &uuid);
> +    g_assert(qemu_uuid_parse(uuidstr, &uuid) == 0);
>

From https://developer.gnome.org/glib/stable/glib-Testing.html#g-assert

The macro can be turned off in final releases of code by defining
G_DISABLE_ASSERT when compiling the application, so code must not depend on
any side effects from expr.

This looks like bad pattern example.

     g_free(uuidstr);
>
>      if (qemu_uuid_is_null(&uuid)) {
>
>
>

Re: [PATCH] spapr: Fix Coverity warning while validating nvdimm options

Posted by Greg Kurz 5 years, 8 months ago

On Wed, 26 Feb 2020 06:10:38 -0600
Shivaprasad G Bhat <sbhat@linux.ibm.com> wrote:

> Fixes Coverity issue,
>       CID 1419883:  Error handling issues  (CHECKED_RETURN)
>            Calling "qemu_uuid_parse" without checking return value
> 
> nvdimm_set_uuid() already verifies if the user provided uuid is valid or
> not. So, need to check for the validity during pre-plug validation again.
> 

Ok but since nvdimm_set_uuid() fills nvdimm->uuid why do you need to parse
the string again in the first place ?

> As this a false positive in this case, assert if not valid to be safe.
> 
> Reported-by: Coverity (CID 1419883)
> Signed-off-by: Shivaprasad G Bhat <sbhat@linux.ibm.com>
> ---
>  hw/ppc/spapr_nvdimm.c |    2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/hw/ppc/spapr_nvdimm.c b/hw/ppc/spapr_nvdimm.c
> index 74eeb8bb74..051727536e 100644
> --- a/hw/ppc/spapr_nvdimm.c
> +++ b/hw/ppc/spapr_nvdimm.c
> @@ -44,7 +44,7 @@ void spapr_nvdimm_validate_opts(NVDIMMDevice *nvdimm, uint64_t size,
>      }
>  
>      uuidstr = object_property_get_str(OBJECT(nvdimm), NVDIMM_UUID_PROP, NULL);
> -    qemu_uuid_parse(uuidstr, &uuid);
> +    g_assert(qemu_uuid_parse(uuidstr, &uuid) == 0);

Like assert(), g_assert() is a macro that can be turned into a nop at
compile time:

#ifdef G_DISABLE_ASSERT
#define g_assert_not_reached()          G_STMT_START { (void) 0; } G_STMT_END
#define g_assert(expr)                  G_STMT_START { (void) 0; } G_STMT_END
#else /* !G_DISABLE_ASSERT */
#define g_assert_not_reached()          G_STMT_START { g_assertion_message_expr (G_LOG_DOMAIN, __FILE__, __LINE__, G_STRFUNC, NULL); } G_STMT_END
#define g_assert(expr)                  G_STMT_START { \
                                             if G_LIKELY (expr) ; else \
                                               g_assertion_message_expr (G_LOG_DOMAIN, __FILE__, __LINE__, G_STRFUNC, \
                                                                         #expr); \
                                        } G_STMT_END
#endif /* !G_DISABLE_ASSERT */

One should avoid putting expressions with side-effects in g_assert() because
the code may not be called at all if G_DISABLE_ASSERT is defined...

>      g_free(uuidstr);
>  
>      if (qemu_uuid_is_null(&uuid)) {

... and uuid would be uninitialized here :-\

If you need to use g_assert(), please do something like:

    ret = qemu_uuid_parse(uuidstr, &uuid);
    g_assert(!ret);

> 
>

Re: [PATCH] spapr: Fix Coverity warning while validating nvdimm options

Posted by Greg Kurz 5 years, 8 months ago

On Wed, 26 Feb 2020 13:49:27 +0100
Greg Kurz <groug@kaod.org> wrote:

> On Wed, 26 Feb 2020 06:10:38 -0600
> Shivaprasad G Bhat <sbhat@linux.ibm.com> wrote:
> 
> > Fixes Coverity issue,
> >       CID 1419883:  Error handling issues  (CHECKED_RETURN)
> >            Calling "qemu_uuid_parse" without checking return value
> > 
> > nvdimm_set_uuid() already verifies if the user provided uuid is valid or
> > not. So, need to check for the validity during pre-plug validation again.
> > 
> 
> Ok but since nvdimm_set_uuid() fills nvdimm->uuid why do you need to parse
> the string again in the first place ?
> 

As discussed on slack, you can forget this remark. Using the QOM accessor
is the way to go.

> > As this a false positive in this case, assert if not valid to be safe.
> > 
> > Reported-by: Coverity (CID 1419883)
> > Signed-off-by: Shivaprasad G Bhat <sbhat@linux.ibm.com>
> > ---
> >  hw/ppc/spapr_nvdimm.c |    2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> > 
> > diff --git a/hw/ppc/spapr_nvdimm.c b/hw/ppc/spapr_nvdimm.c
> > index 74eeb8bb74..051727536e 100644
> > --- a/hw/ppc/spapr_nvdimm.c
> > +++ b/hw/ppc/spapr_nvdimm.c
> > @@ -44,7 +44,7 @@ void spapr_nvdimm_validate_opts(NVDIMMDevice *nvdimm, uint64_t size,
> >      }
> >  
> >      uuidstr = object_property_get_str(OBJECT(nvdimm), NVDIMM_UUID_PROP, NULL);

object_property_get_str() can theoretically return NULL and...

> > -    qemu_uuid_parse(uuidstr, &uuid);

... cause a segv in there because uuidstr will be dereferenced at
some point without checking if it's NULL.

AFAICT there are two scenarios that can cause object_property_get_str()
to return NULL:
- the property doesn't exist
- the property isn't a string

This can probably never happen with the current code base but we
can't about future changes. In order to ensure we abort rather
than segv, I'd pass &error_abort to object_property_get_str().

> > +    g_assert(qemu_uuid_parse(uuidstr, &uuid) == 0);
> 
> Like assert(), g_assert() is a macro that can be turned into a nop at
> compile time:
> 
> #ifdef G_DISABLE_ASSERT
> #define g_assert_not_reached()          G_STMT_START { (void) 0; } G_STMT_END
> #define g_assert(expr)                  G_STMT_START { (void) 0; } G_STMT_END
> #else /* !G_DISABLE_ASSERT */
> #define g_assert_not_reached()          G_STMT_START { g_assertion_message_expr (G_LOG_DOMAIN, __FILE__, __LINE__, G_STRFUNC, NULL); } G_STMT_END
> #define g_assert(expr)                  G_STMT_START { \
>                                              if G_LIKELY (expr) ; else \
>                                                g_assertion_message_expr (G_LOG_DOMAIN, __FILE__, __LINE__, G_STRFUNC, \
>                                                                          #expr); \
>                                         } G_STMT_END
> #endif /* !G_DISABLE_ASSERT */
> 
> One should avoid putting expressions with side-effects in g_assert() because
> the code may not be called at all if G_DISABLE_ASSERT is defined...
> 
> >      g_free(uuidstr);
> >  
> >      if (qemu_uuid_is_null(&uuid)) {
> 
> ... and uuid would be uninitialized here :-\
> 
> If you need to use g_assert(), please do something like:
> 
>     ret = qemu_uuid_parse(uuidstr, &uuid);
>     g_assert(!ret);
> 
> > 
> > 
>

Re: [PATCH] spapr: Fix Coverity warning while validating nvdimm options

Posted by Shivaprasad G Bhat 5 years, 8 months ago

On 02/27/2020 05:58 PM, Greg Kurz wrote:
> On Wed, 26 Feb 2020 13:49:27 +0100
> Greg Kurz <groug@kaod.org> wrote:
>
>>> -    qemu_uuid_parse(uuidstr, &uuid);
> ... cause a segv in there because uuidstr will be dereferenced at
> some point without checking if it's NULL.
>
> AFAICT there are two scenarios that can cause object_property_get_str()
> to return NULL:
> - the property doesn't exist
> - the property isn't a string
>
> This can probably never happen with the current code base but we
> can't about future changes. In order to ensure we abort rather
> than segv, I'd pass &error_abort to object_property_get_str().
Thanks! I just posted the V2 fixing this as well.

Regards,
Shivaprasad