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

[Jan Beulich <jbeulich@xxxxxxxx>]

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 )

?

Jan

>     {
>         if ( dt_prop_cmp(pp->name, name) == 0 )
>         {
>             len = pp->length;
>             break;
>         }
>     }
> 
>     if ( lenp )
>         *lenp = len;
>     return pp;
> }
>