Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::3:5 as permitted sender) client-ip=2620:137:e000::3:5;
Date:   Fri, 29 Sep 2023 10:06:22 +0300
From:   Dan Carpenter <dan.carpenter@linaro.org>
To:     Deepak R Varma <drv@mailo.com>
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
Message-ID: <a2d702b0-e819-47b0-a945-c2e38a162381@kadam.mountain>
References: <ZRXA7n0wD83zhPxC@runicha.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <ZRXA7n0wD83zhPxC@runicha.com>
Precedence: bulk

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