| Message ID | 20201009171307.864608-3-Jerome.Pouiller@silabs.com (mailing list archive) |
|---|---|
| State | Not Applicable |
| Headers | show |
| Series | None | expand |
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.
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).
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
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 --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; }