On Thu, Mar 31, 2022 at 7:35 PM Peter Maydell <peter.mayd...@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.
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;
}
Probably also renaming VISITOR_* to V_* for conciseness. That *should*
quiesce Coverity, and also add some runtime checks.
Paolo
