| Message ID | 20240206073719.3133481-1-alexious@zju.edu.cn (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | media: ttpci: fix two memleaks in budget_av_attach | expand |
On 06/02/2024 08:37, Zhipeng Lu wrote: > When saa7146_register_device and saa7146_vv_init fails, budget_av_attach > should free the resources it allocates, like the error-handling of > ttpci_budget_init does. Besides, there are two fixme comment refers to > such deallocations. > > Fixes: 1da177e4c3f4 ("Linux-2.6.12-rc2") > Signed-off-by: Zhipeng Lu <alexious@zju.edu.cn> > --- > drivers/media/pci/ttpci/budget-av.c | 6 ++++-- > 1 file changed, 4 insertions(+), 2 deletions(-) > > diff --git a/drivers/media/pci/ttpci/budget-av.c b/drivers/media/pci/ttpci/budget-av.c > index 230b104a7cdf..4f897f848581 100644 > --- a/drivers/media/pci/ttpci/budget-av.c > +++ b/drivers/media/pci/ttpci/budget-av.c > @@ -1463,7 +1463,8 @@ static int budget_av_attach(struct saa7146_dev *dev, struct saa7146_pci_extensio > budget_av->has_saa7113 = 1; > err = saa7146_vv_init(dev, &vv_data); > if (err != 0) { > - /* fixme: proper cleanup here */ > + ttpci_budget_deinit(&budget_av->budget); > + kfree(budget_av); > ERR("cannot init vv subsystem\n"); > return err; > } > @@ -1472,7 +1473,8 @@ static int budget_av_attach(struct saa7146_dev *dev, struct saa7146_pci_extensio > vv_data.vid_ops.vidioc_s_input = vidioc_s_input; > > if ((err = saa7146_register_device(&budget_av->vd, dev, "knc1", VFL_TYPE_VIDEO))) { > - /* fixme: proper cleanup here */ > + ttpci_budget_deinit(&budget_av->budget); > + kfree(budget_av); I think this should go after the saa7146_vv_release() release below. Just in case saa7146_vv_release() relies on budget_av somewhere. And besides, it is good practice to release resources in the reverse order they were allocated. Regards, Hans > ERR("cannot register capture v4l2 device\n"); > saa7146_vv_release(dev); > return err;
> On 06/02/2024 08:37, Zhipeng Lu wrote: > > When saa7146_register_device and saa7146_vv_init fails, budget_av_attach > > should free the resources it allocates, like the error-handling of > > ttpci_budget_init does. Besides, there are two fixme comment refers to > > such deallocations. > > > > Fixes: 1da177e4c3f4 ("Linux-2.6.12-rc2") > > Signed-off-by: Zhipeng Lu <alexious@zju.edu.cn> > > --- > > drivers/media/pci/ttpci/budget-av.c | 6 ++++-- > > 1 file changed, 4 insertions(+), 2 deletions(-) > > > > diff --git a/drivers/media/pci/ttpci/budget-av.c b/drivers/media/pci/ttpci/budget-av.c > > index 230b104a7cdf..4f897f848581 100644 > > --- a/drivers/media/pci/ttpci/budget-av.c > > +++ b/drivers/media/pci/ttpci/budget-av.c > > @@ -1463,7 +1463,8 @@ static int budget_av_attach(struct saa7146_dev *dev, struct saa7146_pci_extensio > > budget_av->has_saa7113 = 1; > > err = saa7146_vv_init(dev, &vv_data); > > if (err != 0) { > > - /* fixme: proper cleanup here */ > > + ttpci_budget_deinit(&budget_av->budget); > > + kfree(budget_av); > > ERR("cannot init vv subsystem\n"); > > return err; > > } > > @@ -1472,7 +1473,8 @@ static int budget_av_attach(struct saa7146_dev *dev, struct saa7146_pci_extensio > > vv_data.vid_ops.vidioc_s_input = vidioc_s_input; > > > > if ((err = saa7146_register_device(&budget_av->vd, dev, "knc1", VFL_TYPE_VIDEO))) { > > - /* fixme: proper cleanup here */ > > + ttpci_budget_deinit(&budget_av->budget); > > + kfree(budget_av); > I think this should go after the saa7146_vv_release() release below. > Just in case saa7146_vv_release() relies on budget_av somewhere. And > besides, it is good practice to release resources in the reverse order > they were allocated. Yes, I noticed the order of `kfree(budget_av)` and `ttpci_budget_deinit`, but did forgot the `saa7146_vv_release`. I'll send a v2 version of this patch soon. Regards, Zhipeng > > > ERR("cannot register capture v4l2 device\n"); > > saa7146_vv_release(dev); > > return err;
diff --git a/drivers/media/pci/ttpci/budget-av.c b/drivers/media/pci/ttpci/budget-av.c index 230b104a7cdf..4f897f848581 100644 --- a/drivers/media/pci/ttpci/budget-av.c +++ b/drivers/media/pci/ttpci/budget-av.c @@ -1463,7 +1463,8 @@ static int budget_av_attach(struct saa7146_dev *dev, struct saa7146_pci_extensio budget_av->has_saa7113 = 1; err = saa7146_vv_init(dev, &vv_data); if (err != 0) { - /* fixme: proper cleanup here */ + ttpci_budget_deinit(&budget_av->budget); + kfree(budget_av); ERR("cannot init vv subsystem\n"); return err; } @@ -1472,7 +1473,8 @@ static int budget_av_attach(struct saa7146_dev *dev, struct saa7146_pci_extensio vv_data.vid_ops.vidioc_s_input = vidioc_s_input; if ((err = saa7146_register_device(&budget_av->vd, dev, "knc1", VFL_TYPE_VIDEO))) { - /* fixme: proper cleanup here */ + ttpci_budget_deinit(&budget_av->budget); + kfree(budget_av); ERR("cannot register capture v4l2 device\n"); saa7146_vv_release(dev); return err;
When saa7146_register_device and saa7146_vv_init fails, budget_av_attach should free the resources it allocates, like the error-handling of ttpci_budget_init does. Besides, there are two fixme comment refers to such deallocations. Fixes: 1da177e4c3f4 ("Linux-2.6.12-rc2") Signed-off-by: Zhipeng Lu <alexious@zju.edu.cn> --- drivers/media/pci/ttpci/budget-av.c | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-)