Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Date:   Wed, 20 Apr 2022 10:48:34 -0400
From:   Jaehee Park <jhpark1013@gmail.com>
To:     Dan Carpenter <dan.carpenter@oracle.com>
Cc:     Pavel Skripkin <paskripkin@gmail.com>, Larry.Finger@lwfinger.net,
        phil@philpotter.co.uk, gregkh@linuxfoundation.org,
        linux-staging@lists.linux.dev, linux-kernel@vger.kernel.org,
        outreachy@lists.linux.dev
Subject: Re: [PATCH v3 1/7] staging: r8188eu: remove unused member
 free_bss_buf
Message-ID: <20220420144834.GA1313590@jaehee-ThinkPad-X1-Extreme>
References: <cover.1650392020.git.jhpark1013@gmail.com>
 <3c5a510938ac395a13bb09c0de1868cce8ca3dd8.1650392020.git.jhpark1013@gmail.com>
 <32587233-0ff6-ed0f-b873-cd4f797005a9@gmail.com>
 <20220420115534.GF2951@kadam>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <20220420115534.GF2951@kadam>
Precedence: bulk

On Wed, Apr 20, 2022 at 02:55:34PM +0300, Dan Carpenter wrote:
> As a kernel community, I don't think we are pro-active enough about
> preventing bugs.  One of my co-workers and I have started a bi weekly
> phone call to look at CVEs from the previous week and discuss how they
> could have been prevented or detected earlier.  Of course, my solutions
> are always centered around static analysis because that's my deal but
> some bugs are caused by process issues, or could be detected with better
> testing.
> 
> In this case there were two bugs proposed bugs.
> 
> 1) The memory leak that Fabio noticed.  Smatch is bad at detecting
>    memory leaks.  It's a hard problem, because in this case it's
>    across function boundaries.
> 
> Fabio caught the leak.  I don't know if I would have.
> 
> 2) The NULL dereference.
> 
>    The "&pnetwork->list" expression is not a dereference so this is also
>    a cross function thing.  I thought I used to have an unpublished check
>    for bogus addresses like that where pnetwork is NULL.
> 
>    Another is idea is that when you have pnetwork++ and it's a NULL
>    pointer then print an error message.  Or even potentially NULL.
>    There are various heuristics to use which mean that "A reasonable
>    person would think this could be NULL".
> 
>    Or another idea would be that we could test patches.  Right now we
>    don't really have a way to test these.  But, of course, we wish we
>    did.
> 
> It's not super likely that we would have committed the NULL deref
> patch.  I would have caught that one if you didn't and Fabio likely
> would have as well.  But I like to remove the human error whenever I
> can.
> 
> regards,
> dan carpenter

I'm sorry about the NULL dereference. I wasn't sure about pnetwork
and I should've asked. I wanted to ask why pbuf should be removed
when it was being used for pnetwork but ended up not asking and
sent a faulty patch. Sorry again and thank you for explaining the
errors. I will be more careful about memory leaks and dereferencing
errors. Are there checks that I can run to detect these?

Thanks,
Jaehee