[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [PATCH] spapr: Fix Coverity warning while validating nvdimm options
From: |
Greg Kurz |
Subject: |
Re: [PATCH] spapr: Fix Coverity warning while validating nvdimm options |
Date: |
Thu, 27 Feb 2020 13:28:16 +0100 |
On Wed, 26 Feb 2020 13:49:27 +0100
Greg Kurz <address@hidden> wrote:
> On Wed, 26 Feb 2020 06:10:38 -0600
> Shivaprasad G Bhat <address@hidden> 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 <address@hidden>
> > ---
> > 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);
>
> >
> >
>