Re: use of uninitialized variable involving visit_type_uint32() and friends

[Daniel Henrique Barboza]

On 3/31/22 14:35, Peter Maydell wrote:
> Coverity warns about use of uninitialized data in what seems
> to be a common pattern of use of visit_type_uint32() and similar
> functions. Here's an example from target/arm/cpu64.c:
>
> static void cpu_max_set_sve_max_vq(Object *obj, Visitor *v, const char *name,
>                                     void *opaque, Error **errp)
> {
>      ARMCPU *cpu = ARM_CPU(obj);
>      uint32_t max_vq;
>
>      if (!visit_type_uint32(v, name, &max_vq, errp)) {
>          return;
>      }
>
>      [code that does something with max_vq here]
> }
>
> This doesn't initialize max_vq, on the apparent assumption
> that visit_type_uint32() will do so. But that function is:
>
>
> bool visit_type_uint32(Visitor *v, const char *name, uint32_t *obj,
>                         Error **errp)
> {
>      uint64_t value;
>      bool ok;
>
>      trace_visit_type_uint32(v, name, obj);
>      value = *obj;
>      ok = visit_type_uintN(v, &value, name, UINT32_MAX, "uint32_t", errp);
>      *obj = value;
>      return ok;
> }
>
> So it reads the value of *obj (the uninitialized max_vq).
>
> What's the right way to write this kind of object-property
> setter function? Just pre-initialize the variable to 0?


This reminds me of Valgrind ppc-related warnings I sent patches yesterday. In a
code like this:

(target/ppc/kvm.c)

 int kvmppc_enable_cap_large_decr(PowerPCCPU *cpu, int enable)
 {
    CPUState *cs = CPU(cpu);
    uint64_t lpcr;

    kvm_get_one_reg(cs, KVM_REG_PPC_LPCR_64, &lpcr);
    /* Do we need to modify the LPCR? */
    if (!!(lpcr & LPCR_LD) != !!enable) {

Valgrind complains of "Conditional jump or move depends on uninitialised 
value(s)"
because we're using 'lpcr' in the conditional and 'lpcr' isn't being 
initialized.
Valgrind doesn't seem to care that kvm_get_one_reg() might be writing 'lpcr'.
The fix I proposed consists of initializing the vars in these cases.


My suggestion in this case is to initialize 'max_vq' as well. Apparently these 
static
code analysis tools don't handle the "var being initialized by being passed as 
reference
to another function" scenarios.


Thanks,


Daniel



> thanks
> -- PMM