Re: [PATCH v2] staging: media: atomisp: Fix error path in lm3554_probe()
On 2020-09-03 19:24, Alex Dewar wrote:
The error path for lm3554_probe() contains a number of bugs, including:
* resource leaks
* jumping to error labels out of sequence
* not setting the return value appropriately
Ping?
Fix it up and give the labels more memorable names.
This issue has existed since the code was originally contributed in
commit a49d25364dfb ("staging/atomisp: Add support for the Intel IPU v2"),
although the code was subsequently removed altogether and then
reinstated with commit ad85094b293e ("Revert "media: staging: atomisp: Remove
driver"").
Addresses-Coverity: 1496802 ("Resource leaks")
Fixes: a49d25364dfb ("staging/atomisp: Add support for the Intel IPU v2")
Signed-off-by: Alex Dewar
---
.../media/atomisp/i2c/atomisp-lm3554.c| 53 +++
1 file changed, 30 insertions(+), 23 deletions(-)
diff --git a/drivers/staging/media/atomisp/i2c/atomisp-lm3554.c
b/drivers/staging/media/atomisp/i2c/atomisp-lm3554.c
index 7ca7378b1859..cca10a4c2db0 100644
--- a/drivers/staging/media/atomisp/i2c/atomisp-lm3554.c
+++ b/drivers/staging/media/atomisp/i2c/atomisp-lm3554.c
@@ -833,7 +833,6 @@ static void *lm3554_platform_data_func(struct i2c_client
*client)
static int lm3554_probe(struct i2c_client *client)
{
- int err = 0;
struct lm3554 *flash;
unsigned int i;
int ret;
@@ -843,36 +842,38 @@ static int lm3554_probe(struct i2c_client *client)
return -ENOMEM;
flash->pdata = lm3554_platform_data_func(client);
- if (IS_ERR(flash->pdata))
- return PTR_ERR(flash->pdata);
+ if (IS_ERR(flash->pdata)) {
+ ret = PTR_ERR(flash->pdata);
+ goto err_free_flash;
+ }
v4l2_i2c_subdev_init(>sd, client, _ops);
flash->sd.internal_ops = _internal_ops;
flash->sd.flags |= V4L2_SUBDEV_FL_HAS_DEVNODE;
flash->mode = ATOMISP_FLASH_MODE_OFF;
flash->timeout = LM3554_MAX_TIMEOUT / LM3554_TIMEOUT_STEPSIZE - 1;
- ret =
- v4l2_ctrl_handler_init(>ctrl_handler,
- ARRAY_SIZE(lm3554_controls));
+ ret = v4l2_ctrl_handler_init(>ctrl_handler,
+ARRAY_SIZE(lm3554_controls));
if (ret) {
- dev_err(>dev, "error initialize a ctrl_handler.\n");
- goto fail2;
+ dev_err(>dev, "error initializing ctrl_handler");
+ goto err_unregister_sd;
}
for (i = 0; i < ARRAY_SIZE(lm3554_controls); i++)
v4l2_ctrl_new_custom(>ctrl_handler, _controls[i],
NULL);
- if (flash->ctrl_handler.error) {
- dev_err(>dev, "ctrl_handler error.\n");
- goto fail2;
+ ret = flash->ctrl_handler.error;
+ if (ret) {
+ dev_err(>dev, "ctrl_handler error");
+ goto err_free_ctrl_handler;
}
flash->sd.ctrl_handler = >ctrl_handler;
- err = media_entity_pads_init(>sd.entity, 0, NULL);
- if (err) {
- dev_err(>dev, "error initialize a media entity.\n");
- goto fail1;
+ ret = media_entity_pads_init(>sd.entity, 0, NULL);
+ if (ret) {
+ dev_err(>dev, "error initializing media entity");
+ goto err_free_ctrl_handler;
}
flash->sd.entity.function = MEDIA_ENT_F_FLASH;
@@ -881,20 +882,26 @@ static int lm3554_probe(struct i2c_client *client)
timer_setup(>flash_off_delay, lm3554_flash_off_delay, 0);
- err = lm3554_gpio_init(client);
- if (err) {
+ ret = lm3554_gpio_init(client);
+ if (ret) {
dev_err(>dev, "gpio request/direction_output fail");
- goto fail2;
+ goto err_del_timer;
}
- return atomisp_register_i2c_module(>sd, NULL, LED_FLASH);
-fail2:
+
+ ret = atomisp_register_i2c_module(>sd, NULL, LED_FLASH);
+ if (!ret)
+ return 0;
+
+err_del_timer:
+ del_timer_sync(>flash_off_delay);
media_entity_cleanup(>sd.entity);
+err_free_ctrl_handler:
v4l2_ctrl_handler_free(>ctrl_handler);
-fail1:
+err_unregister_sd:
v4l2_device_unregister_subdev(>sd);
+err_free_flash:
kfree(flash);
-
- return err;
+ return ret;
}
static int lm3554_remove(struct i2c_client *client)
Re: [PATCH v2] staging: media: atomisp: Fix error path in lm3554_probe()
On Thu, Sep 03, 2020 at 07:24:51PM +0100, Alex Dewar wrote: > + > + ret = atomisp_register_i2c_module(>sd, NULL, LED_FLASH); > + if (!ret) > + return 0; Ugh!!! This is a a special case of the "success handling instead of failure handling" anti-pattern where the last condition in the function is different. I just fixed a bug caused by this on Wed. https://www.spinics.net/lists/netdev/msg680226.html But it doesn't cause any problems here so whatever... Reviewed-by: Dan Carpenter regards, dan carpenter
[PATCH v2] staging: media: atomisp: Fix error path in lm3554_probe()
The error path for lm3554_probe() contains a number of bugs, including:
* resource leaks
* jumping to error labels out of sequence
* not setting the return value appropriately
Fix it up and give the labels more memorable names.
This issue has existed since the code was originally contributed in
commit a49d25364dfb ("staging/atomisp: Add support for the Intel IPU v2"),
although the code was subsequently removed altogether and then
reinstated with commit ad85094b293e ("Revert "media: staging: atomisp: Remove
driver"").
Addresses-Coverity: 1496802 ("Resource leaks")
Fixes: a49d25364dfb ("staging/atomisp: Add support for the Intel IPU v2")
Signed-off-by: Alex Dewar
---
.../media/atomisp/i2c/atomisp-lm3554.c| 53 +++
1 file changed, 30 insertions(+), 23 deletions(-)
diff --git a/drivers/staging/media/atomisp/i2c/atomisp-lm3554.c
b/drivers/staging/media/atomisp/i2c/atomisp-lm3554.c
index 7ca7378b1859..cca10a4c2db0 100644
--- a/drivers/staging/media/atomisp/i2c/atomisp-lm3554.c
+++ b/drivers/staging/media/atomisp/i2c/atomisp-lm3554.c
@@ -833,7 +833,6 @@ static void *lm3554_platform_data_func(struct i2c_client
*client)
static int lm3554_probe(struct i2c_client *client)
{
- int err = 0;
struct lm3554 *flash;
unsigned int i;
int ret;
@@ -843,36 +842,38 @@ static int lm3554_probe(struct i2c_client *client)
return -ENOMEM;
flash->pdata = lm3554_platform_data_func(client);
- if (IS_ERR(flash->pdata))
- return PTR_ERR(flash->pdata);
+ if (IS_ERR(flash->pdata)) {
+ ret = PTR_ERR(flash->pdata);
+ goto err_free_flash;
+ }
v4l2_i2c_subdev_init(>sd, client, _ops);
flash->sd.internal_ops = _internal_ops;
flash->sd.flags |= V4L2_SUBDEV_FL_HAS_DEVNODE;
flash->mode = ATOMISP_FLASH_MODE_OFF;
flash->timeout = LM3554_MAX_TIMEOUT / LM3554_TIMEOUT_STEPSIZE - 1;
- ret =
- v4l2_ctrl_handler_init(>ctrl_handler,
- ARRAY_SIZE(lm3554_controls));
+ ret = v4l2_ctrl_handler_init(>ctrl_handler,
+ARRAY_SIZE(lm3554_controls));
if (ret) {
- dev_err(>dev, "error initialize a ctrl_handler.\n");
- goto fail2;
+ dev_err(>dev, "error initializing ctrl_handler");
+ goto err_unregister_sd;
}
for (i = 0; i < ARRAY_SIZE(lm3554_controls); i++)
v4l2_ctrl_new_custom(>ctrl_handler, _controls[i],
NULL);
- if (flash->ctrl_handler.error) {
- dev_err(>dev, "ctrl_handler error.\n");
- goto fail2;
+ ret = flash->ctrl_handler.error;
+ if (ret) {
+ dev_err(>dev, "ctrl_handler error");
+ goto err_free_ctrl_handler;
}
flash->sd.ctrl_handler = >ctrl_handler;
- err = media_entity_pads_init(>sd.entity, 0, NULL);
- if (err) {
- dev_err(>dev, "error initialize a media entity.\n");
- goto fail1;
+ ret = media_entity_pads_init(>sd.entity, 0, NULL);
+ if (ret) {
+ dev_err(>dev, "error initializing media entity");
+ goto err_free_ctrl_handler;
}
flash->sd.entity.function = MEDIA_ENT_F_FLASH;
@@ -881,20 +882,26 @@ static int lm3554_probe(struct i2c_client *client)
timer_setup(>flash_off_delay, lm3554_flash_off_delay, 0);
- err = lm3554_gpio_init(client);
- if (err) {
+ ret = lm3554_gpio_init(client);
+ if (ret) {
dev_err(>dev, "gpio request/direction_output fail");
- goto fail2;
+ goto err_del_timer;
}
- return atomisp_register_i2c_module(>sd, NULL, LED_FLASH);
-fail2:
+
+ ret = atomisp_register_i2c_module(>sd, NULL, LED_FLASH);
+ if (!ret)
+ return 0;
+
+err_del_timer:
+ del_timer_sync(>flash_off_delay);
media_entity_cleanup(>sd.entity);
+err_free_ctrl_handler:
v4l2_ctrl_handler_free(>ctrl_handler);
-fail1:
+err_unregister_sd:
v4l2_device_unregister_subdev(>sd);
+err_free_flash:
kfree(flash);
-
- return err;
+ return ret;
}
static int lm3554_remove(struct i2c_client *client)
--
2.28.0
