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

[Paolo Bonzini <pbonzini@redhat.com>]

On 4/1/22 11:15, Markus Armbruster wrote:
>   +    assert (v->type == expected_type);
>   +    if (expected_type & (VISITOR_INPUT | VISITOR_DEALLOC)) {
> 
> Backwards.

Yes, I always get input vs output wrong.

> 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.

There's a philosophical question on whether other deallocation visitors 
can exist than "the" deallocation visitor, but it's not particularly 
germane to the topic.

> 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...

If it can do really serious interprocedural analysis, it _might_ be able 
to see through the visitor constructor and know that the "value = *obj" 
is not initialized (e.g. "all callers of object_property_set use an 
input visitor").  I doubt that honestly, but a man can dream.

If the conditionals are enough to shut it up, then we won the battle 
(for now).

If the conditionals are not enough to shut it up, then you have a bit 
more confidence when marking the false positives.

>    - Skip *obj = @value when value must be *obj anyway.
> 
>      Should have no observable effect.
> 
>      Again, I'm not sure how this helps static analysis.

Mostly consistency, could also be changed to an assert(*obj == value); 
/* output visitors don't really need obj to be passed by reference */

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

Because it communicates what the caller expects: "I have left this 
uninitialized because I expect my "v" argument to be the kind of visitor 
that fills it in".  It's this argument that gives me the confidence 
needed to shut up Coverity's false positives.

Embedding the visitor type in the signature makes it impossible not to 
pass it, unlike e.g. an assertion in every getter or setter.

Paolo