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

[Luca Fancellu <Luca.Fancellu@xxxxxxx>]

> On 19 Jun 2023, at 09:50, Julien Grall <julien@xxxxxxx> wrote:
> 
> 
> 
> 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?

The fact that we don’t use boolean expressions as conditions (in general, not 
only this example), anyway it was only my personal opinion

> 
> Cheers,
> 
> -- 
> Julien Grall