Re: Potential static analysis ideas

[Hannes Reinecke <hare@suse.de>]

On 7/26/21 10:55 AM, Julia Lawall wrote:
> 
> 
> On Mon, 26 Jul 2021, Arnd Bergmann wrote:
> 
>> On Mon, Jul 26, 2021 at 9:53 AM Geert Uytterhoeven <geert@linux-m68k.org> wrote:
>>> On Mon, Jul 26, 2021 at 9:26 AM Arnd Bergmann <arnd@arndb.de> wrote:
>>>> On Sun, Jul 25, 2021 at 1:45 AM NeilBrown <neilb@suse.de> wrote:
>>>>> On Sun, 25 Jul 2021, Laurent Pinchart wrote:
>>>>>>> To make it work well, you need to know if frob() and/or the current
>>>>>>> function return an error code or not.  While you can use some heuristics
>>>>>>> (e.g. is there any return -Exxx), perhaps we can add an annotation to
>>>>>>> indicate if a function returns an error code, or an error pointer?
>>>>>>
>>>>>> https://lore.kernel.org/linux-media/YNMvarFl%2FKU1pGCG@pendragon.ideasonboard.com/
>>>>>>
>>>>>> I think it would be useful, if not for the tools, at least for
>>>>>> developers.
>>>>>
>>>>> Agreed.  I added some code to smatch so that I could annotate pointers to
>>>>> say if they are allowed to be NULL.  The implementation isn't perfect,
>>>>> but I love having that extra documentation about when I do or don't have
>>>>> to check for NULL.
>>>>
>>>> I can think of four different annotations that limit what a pointer return from
>>>> a function can be:
>>>>
>>>> a) either a valid pointer or NULL, but never an error pointer,
>>>> b) either a valid pointer or an error pointer, but not NULL,
>>>> c) always a valid pointer, never NULL or an error,
>>>> d) always NULL, but callers are expected to check for error pointers.
>>>
>>> e) either a valid pointer, NULL, or an error pointer
>>>
>>> The last pattern is seen with the various *get*_optional() functions.
>>
>> I would always consider those the exact bug that I meant with "because
>> everyone gets those wrong". I think the idea of the "optional" functions is
>> that you have two implementations b) and d) and pick one of them
>> at compile time. To the caller this means either an error pointer or
>> success, but checking for NULL is a bug in the caller, while conditionally
>> returning NULL or ERR_PTR() would be a bug in the interface.
> 
> I'm not sure to understand the "bug in the caller" part.  Couldn't there
> be two possible definitions of the called function, according to different
> configuration options, and a single caller that calls both?
> 
> Also, over 230 files contain functions that return both NULL and ERR_PTR.
> A random example, chosen for conciseness, is the following from
> fs/overlayfs/inode.c:
> 
> struct inode *ovl_lookup_inode(struct super_block *sb, struct dentry *real,
>                                 bool is_upper)
> {
>   	struct inode *inode, *key = d_inode(real);
> 
>          inode = ilookup5(sb, (unsigned long) key, ovl_inode_test, key);
> 	if (!inode)
>                  return NULL;
> 
>          if (!ovl_verify_inode(inode, is_upper ? NULL : real,
>                                is_upper ? real : NULL, false)) {
>                  iput(inode);
>                  return ERR_PTR(-ESTALE);
>          }
> 
>          return inode;
> }
> 
And that I would consider a coding error.
If a function is able to return an error pointer it should _always_ 
return an error pointer; here it would be trivial to return -ENXIO 
instead of NULL in the first condition.

Not doing so is just sloppy programming IMO.

Cheers,

Hannes
-- 
Dr. Hannes Reinecke                Kernel Storage Architect
hare@suse.de                              +49 911 74053 688
SUSE Software Solutions GmbH, Maxfeldstr. 5, 90409 Nürnberg
HRB 36809 (AG Nürnberg), Geschäftsführer: Felix Imendörffer