* [PATCH] spapr: Fix Coverity warning while validating nvdimm options
@ 2020-02-26 12:10 Shivaprasad G Bhat
2020-02-26 12:27 ` Philippe Mathieu-Daudé
2020-02-26 12:49 ` Greg Kurz
0 siblings, 2 replies; 5+ messages in thread
From: Shivaprasad G Bhat @ 2020-02-26 12:10 UTC (permalink / raw)
To: qemu-ppc, david; +Cc: peter.maydell, qemu-devel
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)) {
^ permalink raw reply related [flat|nested] 5+ messages in thread
* Re: [PATCH] spapr: Fix Coverity warning while validating nvdimm options
2020-02-26 12:10 [PATCH] spapr: Fix Coverity warning while validating nvdimm options Shivaprasad G Bhat
@ 2020-02-26 12:27 ` Philippe Mathieu-Daudé
2020-02-26 12:49 ` Greg Kurz
1 sibling, 0 replies; 5+ messages in thread
From: Philippe Mathieu-Daudé @ 2020-02-26 12:27 UTC (permalink / raw)
To: Shivaprasad G Bhat
Cc: Peter Maydell, qemu-ppc@nongnu.org list:PowerPC,
qemu-devel@nongnu.org Developers, David Gibson
[-- Attachment #1: Type: text/plain, Size: 1496 bytes --]
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)) {
>
>
>
[-- Attachment #2: Type: text/html, Size: 3131 bytes --]
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH] spapr: Fix Coverity warning while validating nvdimm options
2020-02-26 12:10 [PATCH] spapr: Fix Coverity warning while validating nvdimm options Shivaprasad G Bhat
2020-02-26 12:27 ` Philippe Mathieu-Daudé
@ 2020-02-26 12:49 ` Greg Kurz
2020-02-27 12:28 ` Greg Kurz
1 sibling, 1 reply; 5+ messages in thread
From: Greg Kurz @ 2020-02-26 12:49 UTC (permalink / raw)
To: Shivaprasad G Bhat; +Cc: peter.maydell, qemu-ppc, qemu-devel, david
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);
>
>
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH] spapr: Fix Coverity warning while validating nvdimm options
2020-02-26 12:49 ` Greg Kurz
@ 2020-02-27 12:28 ` Greg Kurz
2020-02-27 13:44 ` Shivaprasad G Bhat
0 siblings, 1 reply; 5+ messages in thread
From: Greg Kurz @ 2020-02-27 12:28 UTC (permalink / raw)
To: Shivaprasad G Bhat; +Cc: peter.maydell, qemu-ppc, qemu-devel, david
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);
>
> >
> >
>
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH] spapr: Fix Coverity warning while validating nvdimm options
2020-02-27 12:28 ` Greg Kurz
@ 2020-02-27 13:44 ` Shivaprasad G Bhat
0 siblings, 0 replies; 5+ messages in thread
From: Shivaprasad G Bhat @ 2020-02-27 13:44 UTC (permalink / raw)
To: Greg Kurz; +Cc: peter.maydell, qemu-ppc, qemu-devel, david
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
^ permalink raw reply [flat|nested] 5+ messages in thread
end of thread, other threads:[~2020-02-27 13:45 UTC | newest]
Thread overview: 5+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-02-26 12:10 [PATCH] spapr: Fix Coverity warning while validating nvdimm options Shivaprasad G Bhat
2020-02-26 12:27 ` Philippe Mathieu-Daudé
2020-02-26 12:49 ` Greg Kurz
2020-02-27 12:28 ` Greg Kurz
2020-02-27 13:44 ` Shivaprasad G Bhat
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.