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

[Jan Beulich <jbeulich@xxxxxxxx>]

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, ...

> My proposal, in a future patch, is to refactor these kinds of functions 
> as follows:
> 
> 
> 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;
> 
>      if ( !np )
>          return NULL;

... this path would be a problem as well.

>      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;
> }
> 
> 
> The advantage here is that we can easily argue that `*lenp' is always
> initialized by the function (if not NULL) and inform the tool about
> this, which is a safer API and also resolves almost all subsequent
> "don't know"s about further uses of the variables involved (e.g. `lenp').

The disadvantage is that in a more complex case and with the function
e.g. being static, the initializer of "len" may prevent compiler /
tools from spotting cases where the variable would (otherwise) truly
(and wrongly) remain uninitialized (and that fact propagating up the
call chain, through - in this example - whatever variable's address
the caller passed for "lenp"). IOW - I don't think a common pattern
can be agreed upon up front for cases like this one.

Jan