On 16/06/23 09:19, Jan Beulich wrote:
On 15.06.2023 18:39, nicola wrote:
while investigating possible patches regarding Mandatory Rule 9.1, I
found the following pattern, that is likely to results in a lot possible
positives from many (all) static analysis tools for this rule.
This is the current status (taken from `xen/common/device_tree.c:135')
const struct dt_property *dt_find_property(const struct dt_device_node *np,
const char *name, u32 *lenp)
{
const struct dt_property *pp;
if ( !np )
return NULL;
for ( pp = np->properties; pp; pp = pp->next )
{
if ( dt_prop_cmp(pp->name, name) == 0 )
{
if ( lenp )
*lenp = pp->length;
break;
}
}
return pp;
}
It's very hard to detect that the pointee is always written whenever a
non-NULL pointer for `lenp' is supplied, and it can safely be read in
the callee, so a sound analysis will err on the cautious side.
I'm having trouble seeing why this is hard to recognize: The loop can
only be exited two ways: pp == NULL or with *lenp written.
For rule 9.1 I'd rather expect the scanning tool (and often the compiler)
to get into trouble with the NULL return value case, and *lenp not being
written yet apparently consumed in the caller. Then, however, ...
You're right, I made a mistake, thank you for finding it.
I meant to write on `*lenp' in all execution paths.
Please, take a look at this revised version:
const struct dt_property *dt_find_property(const struct dt_device_node *np,
const char *name, u32 *lenp)
{
u32 len = 0;
const struct dt_property *pp = NULL;
if ( np )
{
for ( pp = np->properties; pp; pp = pp->next )
{
if ( dt_prop_cmp(pp->name, name) == 0 )
{
len = pp->length;
break;
}
}
}
if ( lenp )
*lenp = len;
return pp;
}
My proposal, in a future patch, is to refactor these kinds of functions
as follows:
const struct dt_property *dt_find_property(const struct dt_device_node *np,
const char *name, u32 *lenp)
{
u32 len = 0;
const struct dt_property *pp;
if ( !np )
return NULL;
... this path would be a problem as well.
for ( pp = np->properties; pp; pp = pp->next )
{
if ( dt_prop_cmp(pp->name, name) == 0 )
{
len = pp->length;
break;
}
}
if ( lenp )
*lenp = len;
return pp;
}
The advantage here is that we can easily argue that `*lenp' is always
initialized by the function (if not NULL) and inform the tool about
this, which is a safer API and also resolves almost all subsequent
"don't know"s about further uses of the variables involved (e.g. `lenp')
The disadvantage is that in a more complex case and with the function
e.g. being static, the initializer of "len" may prevent compiler /
tools from spotting cases where the variable would (otherwise) truly
(and wrongly) remain uninitialized (and that fact propagating up the
call chain, through - in this example - whatever variable's address
the caller passed for "lenp"). IOW - I don't think a common pattern
can be agreed upon up front for cases like this one.
That's ok, but perhaps we can agree that in a subset of functions as
simple as this one the refactoring can help both developers and tools.
--
Nicola Vetrini, BSc
Software Engineer, BUGSENG srl (https://bugseng.com)
