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

[Luca Fancellu <Luca.Fancellu@xxxxxxx>]

> 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,
which I dislike.

Anyway I also think that it’s difficult to have a generic rule for cases like 
that, also because for some function
maybe the author intention was to don’t write the *lenp in case some error 
occur before, anyway this can
be easily made clear adding documentation to the function, for these cases.


> 
> Cheers,
> 
> -- 
> Julien Grall
>