diff mbox series

gfs2: fix 'passing zero to ERR_PTR()' warning

Message ID ZRXA7n0wD83zhPxC@runicha.com
State New
Headers
Series gfs2: fix 'passing zero to ERR_PTR()' warning |

Commit Message

Deepak R Varma Sept. 28, 2023, 6:07 p.m. UTC 
  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>
---
Please note: Patch is build tested only.


 fs/gfs2/acl.c | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

--
2.39.2

Comments

Dan Carpenter Sept. 29, 2023, 7:06 a.m. UTC | #1 
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
Deepak R Varma Sept. 29, 2023, 7:39 a.m. UTC | #2 
On Fri, 2023-09-29 at 10:06 +0300, Dan Carpenter wrote:
> 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.
> 
Hi Dan,
Thank you for the review, feedback and guidance. This is really
helpful.

regards,
deepak.

> regards,
> dan carpenter
>
diff mbox series

Patch

diff --git a/fs/gfs2/acl.c b/fs/gfs2/acl.c
index 443640e6fb9c..818d67f10dcf 100644
--- a/fs/gfs2/acl.c
+++ b/fs/gfs2/acl.c
@@ -50,8 +50,11 @@  static struct posix_acl *__gfs2_get_acl(struct inode *inode, int type)

 	name = gfs2_acl_name(type);
 	len = gfs2_xattr_acl_get(ip, name, &data);
-	if (len <= 0)
+	if (len == 0)
+		return NULL;
+	if (len < 0)
 		return ERR_PTR(len);
+
 	acl = posix_acl_from_xattr(&init_user_ns, data, len);
 	kfree(data);
 	return acl;