Re: Refactoring of a possibly unsafe pattern for variable initialization via function calls

[Stefano Stabellini <sstabellini@xxxxxxxxxx>]

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

    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;
}