From: drv <drv@mailo.com>
To: Dan Carpenter <dan.carpenter@linaro.org>
Cc: Bob Peterson <rpeterso@redhat.com>,
Andreas Gruenbacher <agruenba@redhat.com>,
gfs2@lists.linux.dev, linux-kernel@vger.kernel.org,
linux-kernel-mentees@lists.linuxfoundation.org,
Dan Carpenter <error27@gmail.com>
Subject: Re: [PATCH] gfs2: fix 'passing zero to ERR_PTR()' warning
Date: Fri, 29 Sep 2023 13:09:11 +0530 [thread overview]
Message-ID: <0c1ed836c5a306331e1b2a97217c3c9f7e1fb701.camel@mailo.com> (raw)
In-Reply-To: <a2d702b0-e819-47b0-a945-c2e38a162381@kadam.mountain>
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
>
prev parent reply other threads:[~2023-09-29 7:39 UTC|newest]
Thread overview: 2+ messages / expand[flat|nested] mbox.gz Atom feed top
[not found] <ZRXA7n0wD83zhPxC@runicha.com>
2023-09-29 7:06 ` [PATCH] gfs2: fix 'passing zero to ERR_PTR()' warning Dan Carpenter
2023-09-29 7:39 ` drv [this message]
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=0c1ed836c5a306331e1b2a97217c3c9f7e1fb701.camel@mailo.com \
--to=drv@mailo.com \
--cc=agruenba@redhat.com \
--cc=dan.carpenter@linaro.org \
--cc=error27@gmail.com \
--cc=gfs2@lists.linux.dev \
--cc=linux-kernel-mentees@lists.linuxfoundation.org \
--cc=linux-kernel@vger.kernel.org \
--cc=rpeterso@redhat.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).