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

[Greg Kurz]

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

> 
>