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

[Greg Kurz <groug@kaod.org>]

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