Messages in this thread | | | Date | Fri, 29 Sep 2023 10:06:22 +0300 | From | Dan Carpenter <> | Subject | Re: [PATCH] gfs2: fix 'passing zero to ERR_PTR()' warning |
| |
On Thu, Sep 28, 2023 at 11:37:42PM +0530, Deepak R Varma wrote: > Resolve the following Smatch static checker warning: > fs/gfs2/acl.c:54 __gfs2_get_acl() warn: passing zero to 'ERR_PTR' > > by returning NULL when an extended attribute length is zero, instead of > passing on zero to the ERR_PTR(). > > Signed-off-by: Deepak R Varma <drv@mailo.com> > ---
Passing zero to ERR_PTR() is not a bug.
You're patch doesn't change how the code works at all, right? So it's like a cleanup patch. But the code was nicer in the original.
This is just a false positive. Ignore static checker false positives. Fix the checker instead. Although in this case, I can't think of an easy way fix the checker. Perhaps don't print a warning if the callers check for NULL?
The passing zero to ERR_PTR() warning is actually a pretty good heuristic. 90% of the time in new code this is a real bug. But in old code then probably it's 0% real bugs because we've been reviewing these warnings for over a decade.
I have a blog which might be useful. https://staticthinking.wordpress.com/2022/08/01/mixing-error-pointers-and-null/
When I'm reviewing this patch I think: 1) Does gfs2_xattr_acl_get() return zero? And it does. 2) Does that look intentional. It's harder to tell because there aren't comments and it looks like it might be a missing error code. But when you read it closely then actually it does look intentional. In terms of Smatch, I consider it "intentional" if there is an "error = 0;" within 5 lines for the goto. (Other languages like Rust are better than C because they force everyone to follow the rules. #trolling). 3) Do the callers of __gfs2_get_acl() check for NULL and they do.
So this code is fine.
I hope this helps you in your review process. 1) Ignore old warnings. 2) Ignore false positives. 3) If you think it is a bug, then try to figure out how it will cause a crash. Look at the caller etc.
regards, dan carpenter
| |