diff mbox series

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

Message ID 20201009171307.864608-3-Jerome.Pouiller@silabs.com (mailing list archive)
State Not Applicable
Headers show
Series None | expand

Commit Message

Jérôme Pouiller Oct. 9, 2020, 5:13 p.m. UTC
From: Jérôme Pouiller <jerome.pouiller@silabs.com>

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@oracle.com>
Signed-off-by: Jérôme Pouiller <jerome.pouiller@silabs.com>
---
 drivers/staging/wfx/main.c | 8 +++++++-
 1 file changed, 7 insertions(+), 1 deletion(-)

Comments

Kalle Valo Oct. 9, 2020, 6:51 p.m. UTC | #1
Jerome Pouiller <Jerome.Pouiller@silabs.com> writes:

> From: Jérôme Pouiller <jerome.pouiller@silabs.com>
>
> 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@oracle.com>
> Signed-off-by: Jérôme Pouiller <jerome.pouiller@silabs.com>
> ---
>  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.
Jérôme Pouiller Oct. 10, 2020, 12:07 p.m. UTC | #2
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@silabs.com> writes:
> 
> > From: Jérôme Pouiller <jerome.pouiller@silabs.com>
> >
> > 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@oracle.com>
> > Signed-off-by: Jérôme Pouiller <jerome.pouiller@silabs.com>
> > ---
> >  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.

Greg has already applied the series, but I don't forget this review. I
will take it into account in the series I am going to send you (probably
in the v2, in order to not defer the v1).
Dan Carpenter Oct. 10, 2020, 1:18 p.m. UTC | #3
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@silabs.com> writes:
> > 
> > > From: Jérôme Pouiller <jerome.pouiller@silabs.com>
> > >
> > > 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@oracle.com>
> > > Signed-off-by: Jérôme Pouiller <jerome.pouiller@silabs.com>
> > > ---
> > >  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;"

These sorts of rules where "there is only one return per function" are
meant to make people think about cleanup before returning.  But most of
my work is in error handling code and it doesn't help.  If people don't
think about cleanup, changing the style won't make them start thinking
about it.  There was one driver which was written with locked code
indented one tab and the inventor of that style still introduced a
locking bug in his code.

	spin_lock(); {
		frob();
		frob();
		if (ret)
			return ret;  // <-- forgot to unlock;
		frob();
	} spin_unlock();

Btw, I have created a new Smatch check to find unwind bugs.  It's called
check_unwind.c and it's easy to add new alloc/free pairings to that
code.  This is the best way to prevent unwind bugs.  The style changes
don't make a measurable difference in real life and they make the code
messy.

And GW-BASIC label names are a pox upon the earth.

regards,
dan carpenter
Dan Carpenter Oct. 10, 2020, 1:54 p.m. UTC | #4
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@silabs.com> writes:
> > > 
> > > > From: Jérôme Pouiller <jerome.pouiller@silabs.com>
> > > >
> > > > 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@oracle.com>
> > > > Signed-off-by: Jérôme Pouiller <jerome.pouiller@silabs.com>
> > > > ---
> > > >  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
diff mbox series

Patch

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;
 }