Re: [PATCH 2/8] staging: wfx: check memory allocation

[Dan Carpenter]

On Sat, Oct 10, 2020 at 04:18:11PM +0300, Dan Carpenter wrote:
> On Sat, Oct 10, 2020 at 02:07:13PM +0200, Jérôme Pouiller wrote:
> > On Friday 9 October 2020 20:51:01 CEST Kalle Valo wrote:
> > > CAUTION: This email originated from outside of the organization. Do not click links or open attachments unless you recognize the sender and know the content is safe.
> > > 
> > > 
> > > Jerome Pouiller <Jerome.Pouiller@xxxxxxxxxx> writes:
> > > 
> > > > From: Jérôme Pouiller <jerome.pouiller@xxxxxxxxxx>
> > > >
> > > > Smatch complains:
> > > >
> > > >    main.c:228 wfx_send_pdata_pds() warn: potential NULL parameter dereference 'tmp_buf'
> > > >    227          tmp_buf = kmemdup(pds->data, pds->size, GFP_KERNEL);
> > > >    228          ret = wfx_send_pds(wdev, tmp_buf, pds->size);
> > > >                                          ^^^^^^^
> > > >    229          kfree(tmp_buf);
> > > >
> > > > Reported-by: Dan Carpenter <dan.carpenter@xxxxxxxxxx>
> > > > Signed-off-by: Jérôme Pouiller <jerome.pouiller@xxxxxxxxxx>
> > > > ---
> > > >  drivers/staging/wfx/main.c | 8 +++++++-
> > > >  1 file changed, 7 insertions(+), 1 deletion(-)
> > > >
> > > > diff --git a/drivers/staging/wfx/main.c b/drivers/staging/wfx/main.c
> > > > index df11c091e094..a8dc2c033410 100644
> > > > --- a/drivers/staging/wfx/main.c
> > > > +++ b/drivers/staging/wfx/main.c
> > > > @@ -222,12 +222,18 @@ static int wfx_send_pdata_pds(struct wfx_dev *wdev)
> > > >       if (ret) {
> > > >               dev_err(wdev->dev, "can't load PDS file %s\n",
> > > >                       wdev->pdata.file_pds);
> > > > -             return ret;
> > > > +             goto err1;
> > > >       }
> > > >       tmp_buf = kmemdup(pds->data, pds->size, GFP_KERNEL);
> > > > +     if (!tmp_buf) {
> > > > +             ret = -ENOMEM;
> > > > +             goto err2;
> > > > +     }
> > > >       ret = wfx_send_pds(wdev, tmp_buf, pds->size);
> > > >       kfree(tmp_buf);
> > > > +err2:
> > > >       release_firmware(pds);
> > > > +err1:
> > > >       return ret;
> > > >  }
> > > 
> > > A minor style issue but using more descriptive error labels make the
> > > code more readable and maintainable, especially in a bigger function.
> > > For example, err2 could be called err_release_firmware.
> > > 
> > > And actually err1 could be removed and the goto replaced with just
> > > "return ret;". Then err2 could be renamed to a simple err.
> > 
> > It was the case in the initial code. However, I have preferred to not
> > mix 'return' and 'goto' inside the same function. Probably a matter of
> > taste.
> >
> 
> Ideally you can read a function from top to bottom and understand with
> out skipping around.  Imagine if novels were written like that "goto
> bottom_of_page;" but then at the bottom it just said "Just kidding".
> "return ret;" is more readable than "goto err;"

More unasked for exposition:  "goto err;" is too vague.  It could be one
of three things.  1)  Do nothing (like this code).  2)  Do something
specific (choose a better name like goto unlock).  3) Do everything.
Do everything code is the most buggy style of error handling.

The common bug introduced by type 1 and 2 are "Forgot to set the error
code" bugs.  Type 3 is a whole nother level of bugginess.  Too much bugs
to explain.

regards,
dan carpenter