Re: [PATCH] mm: hugetlb: checking for IS_ERR() instead of NULL

[Nigel Christian <nigel.l.christian@gmail.com>]

On Wed, Jun 02, 2021 at 05:47:52PM +0300, Dan Carpenter wrote:
> On Tue, Jun 01, 2021 at 11:50:06PM +0300, Dan Carpenter wrote:
> > On Tue, Jun 01, 2021 at 10:51:23PM +0300, Dan Carpenter wrote:
> > > The other thing which might be interesting is if you pass a NULL
> > > to IS_ERR() and then dereference the NULL then print a warning about
> > > that.  This has a lot of overlaps with some of my existing checks, but
> > > it's still a new idea so it belongs in a separate check.  It's fine and
> > > good even if one bug triggers a lot of different warnings.  I'll write
> > > that, hang on, brb.
> > 
> > 100% untested.  :)  I'll test it tonight.
> > 
> 
> This test is decent, but I ended up making a few changes:
> 
> 1)  My devel version of Smatch had a new bug in it which caused some
>     false positives.  Fixed now, hopefully.
  
Many thanks! Gonna check today.

> 
> 2)  The test:
> 
> 	if (get_state_expr(my_id, expr) != &null)
> 		return;
> 
>     check was not strict enough.  I realized that I knew that from
>     square one but I was lazy.  So now I have introduced a global helper
>     function and updated the code:
> 
> bool expr_has_possible_state(int owner, struct expression *expr, struct smatch_state *state)
> {
>         struct sm_state *sm;
> 
>         sm = get_sm_state_expr(owner, expr);
>         if (!sm)
>                 return false;
> 
>         return slist_has_state(sm->possible, state);
> }
> 
>     I replaced the test with:
> 
> 	if (!expr_has_possible_state(my_id, expr, &null))
> 
> 3)  The warning message was too vague and too similar to other warning
>     messages.  It should be something unique to the test.  It's now:
> 
> 	sm_error("potential NULL/IS_ERR bug '%s'", name);
> 
> I'll post the results tomorrow.

This is great, will be a nice bug to check for beginners like me.

> 
> regards,
> dan carpenter
>