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
