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

[Markus Armbruster <armbru@redhat.com>]

Paolo Bonzini <pbonzini@redhat.com> writes:

> On Thu, Mar 31, 2022 at 7:35 PM Peter Maydell <peter.maydell@linaro.org>
> 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 [...]
>> reads the value of *obj (the uninitialized max_vq).
>>
>
> The visit_type_* functions are written to work for both getters and setters.

Yes.

This is convenient for uses that are actually visitor-agnostic, such as
the generated qapi-visit-FOO.c

It can be really ugly for output-only uses.  In particular for strings,
where we have to pass a char ** instead of a const char *.

> For the leaves, that means potentially reading uninitialized data.  It is
> harmless but very ugly, and with respect to static analysis it was all but
> a time bomb, all the time.
>
> The best (but most intrusive) solution would be to add a parameter to all
> visit_type_* functions with the expected "direction" of the visit, which
> could be checked against v->type.
>
> That is:
>
> bool visit_type_uint32(VisitorType expected_type, Visitor *v, const char
> *name, uint32_t *obj,
>                        Error **errp)
> {
>     uint64_t value;
>     bool ok;
>
>     trace_visit_type_uint32(v, name, obj);
>     assert (v->type == expected_type);
>     if (expected_type & (VISITOR_INPUT | VISITOR_DEALLOC)) {
>         value = *obj;
>     }
>     ok = visit_type_uintN(v, &value, name, UINT32_MAX, "uint32_t", errp);
>     assert (ok || expected_type == VISITOR_INPUT);
>     if (expected_type & VISITOR_OUTPUT) {
>         *obj = value;
>     }
>     return ok;
> }

As diff -w:

 -bool visit_type_uint32(Visitor *v, const char *name, uint32_t *obj,
 +bool visit_type_uint32(VisitorType expected_type, Visitor *v, const char
 +*name, uint32_t *obj,
                         Error **errp)
  {
      uint64_t value;
      bool ok;

      trace_visit_type_uint32(v, name, obj);
 +    assert (v->type == expected_type);
 +    if (expected_type & (VISITOR_INPUT | VISITOR_DEALLOC)) {

Backwards.

With an input visitor @v,

    visit_type_uint32(v, "name", &val, errp)

stores to @val without looking at it first.  In other words,
uninitialized @val is fine, just like for val = ...

Note: you don't actually need VISITOR_DEALLOC here, because a
deallocation visitor isn't going to do anything for non-pointer values.

With an output visitor @v,

    visit_type_uint32(v, "name", &val, errp)

reads from @val without changing it.

      value = *obj;
 +    }
      ok = visit_type_uintN(v, &value, name, UINT32_MAX, "uint32_t", errp);
 +    assert (ok || expected_type == VISITOR_INPUT);
 +    if (expected_type & VISITOR_OUTPUT) {

Also backwards.  

      *obj = value;
 +    }
      return ok;
  }

Two changes:

* Skip copying to and from full-width buffer @value:

  - Skip @value = *obj when we're going to overwrite @value without
    reading it first.

    This leaves @value uninitialized instead of initializing it from a
    (commonly) uninitialized variable.

    I'm not sure how this helps static analysis, but if it does...

  - Skip *obj = @value when value must be *obj anyway.

    Should have no observable effect.

    Again, I'm not sure how this helps static analysis.

  Note that only the functions for types narrower than 64 bits have such
  copying code.  Skipping the assignments creates a tiny inconsistency
  between narrow and fill-width visits.

* Pass visitor type in addition to the visitor.  Can you explain why
  that's useful?

> Probably also renaming VISITOR_* to V_* for conciseness.  That *should*
> quiesce Coverity, and also add some runtime checks.
>
> Paolo