[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: Refactoring of a possibly unsafe pattern for variable initialization via function calls
On 19/06/2023 09:31, Luca Fancellu wrote:
On 19 Jun 2023, at 09:23, Julien Grall <julien@xxxxxxx> wrote:
On 19/06/2023 09:18, Jan Beulich wrote:
On 16.06.2023 22:56, Stefano Stabellini wrote:
On Fri, 16 Jun 2023, Nicola Vetrini wrote:
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;
}
Nesting more will make the code less readable and also cause other code
quality metrics to deteriorate (cyclomatic complexity).
Would the below work?
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 )
return NULL
That's what we started from, but leaving *lenp not written to. How
about ...
for ( pp = np->properties; pp; pp = pp->next )
for ( pp = np ? np->properties : NULL; pp; pp = pp->next ) > > ?
I would be OK with that. Maybe with an extra set of parentheses around ' np ?
... : NULL' just to make visually easier to parse.
Agree, and for MISRA, we should use a boolean expression as condition, even if
I know that we would like to deviate from that,
The code will even be more difficult to read. So if we plan to deviate,
then I don't want us to use MISRA-compliant boolean expression here.
which I dislike.
What do you dislike?
Cheers,
--
Julien Grall
|
