* [PATCH] rc: correctly handling failed allocation
@ 2016-02-16 2:33 Insu Yun
2016-02-16 10:54 ` Sean Young
0 siblings, 1 reply; 3+ messages in thread
From: Insu Yun @ 2016-02-16 2:33 UTC (permalink / raw)
To: sean, mchehab, linux-media, linux-kernel
Cc: taesoo, yeongjin.jang, insu, changwoo, Insu Yun
Since rc_allocate_device() uses kmalloc,
it can returns NULL, so need to check,
otherwise, NULL derefenrece can be happened.
Signed-off-by: Insu Yun <wuninsu@gmail.com>
---
drivers/media/rc/igorplugusb.c | 3 +++
1 file changed, 3 insertions(+)
diff --git a/drivers/media/rc/igorplugusb.c b/drivers/media/rc/igorplugusb.c
index b36e515..df37cd5 100644
--- a/drivers/media/rc/igorplugusb.c
+++ b/drivers/media/rc/igorplugusb.c
@@ -191,6 +191,8 @@ static int igorplugusb_probe(struct usb_interface *intf,
usb_make_path(udev, ir->phys, sizeof(ir->phys));
rc = rc_allocate_device();
+ if (!rc)
+ goto fail;
rc->input_name = DRIVER_DESC;
rc->input_phys = ir->phys;
usb_to_input_id(udev, &rc->input_id);
@@ -213,6 +215,7 @@ static int igorplugusb_probe(struct usb_interface *intf,
ir->rc = rc;
ret = rc_register_device(rc);
if (ret) {
+fail:
dev_err(&intf->dev, "failed to register rc device: %d", ret);
rc_free_device(rc);
usb_free_urb(ir->urb);
--
1.9.1
^ permalink raw reply related [flat|nested] 3+ messages in thread
* Re: [PATCH] rc: correctly handling failed allocation
2016-02-16 2:33 [PATCH] rc: correctly handling failed allocation Insu Yun
@ 2016-02-16 10:54 ` Sean Young
2016-03-03 13:02 ` Mauro Carvalho Chehab
0 siblings, 1 reply; 3+ messages in thread
From: Sean Young @ 2016-02-16 10:54 UTC (permalink / raw)
To: Insu Yun
Cc: mchehab, linux-media, linux-kernel, taesoo, yeongjin.jang, insu,
changwoo
On Mon, Feb 15, 2016 at 09:33:11PM -0500, Insu Yun wrote:
> Since rc_allocate_device() uses kmalloc,
> it can returns NULL, so need to check,
> otherwise, NULL derefenrece can be happened.
Thanks for catching that.
> Signed-off-by: Insu Yun <wuninsu@gmail.com>
> ---
> drivers/media/rc/igorplugusb.c | 3 +++
> 1 file changed, 3 insertions(+)
>
> diff --git a/drivers/media/rc/igorplugusb.c b/drivers/media/rc/igorplugusb.c
> index b36e515..df37cd5 100644
> --- a/drivers/media/rc/igorplugusb.c
> +++ b/drivers/media/rc/igorplugusb.c
> @@ -191,6 +191,8 @@ static int igorplugusb_probe(struct usb_interface *intf,
> usb_make_path(udev, ir->phys, sizeof(ir->phys));
>
> rc = rc_allocate_device();
> + if (!rc)
> + goto fail;
At this point, ret is not initialized but will be used in the error path.
> rc->input_name = DRIVER_DESC;
> rc->input_phys = ir->phys;
> usb_to_input_id(udev, &rc->input_id);
> @@ -213,6 +215,7 @@ static int igorplugusb_probe(struct usb_interface *intf,
> ir->rc = rc;
> ret = rc_register_device(rc);
> if (ret) {
I'm not sure how common it is to goto into another nesting level for an
error path. Also I just noticed that the code is leaking the timer in
the error path.
It might be better to put the "fail:" at the end after the last return
for the successful case, and have a goto to it after both
rc_allocate_device() and rc_register_device() in case they fail.
> +fail:
> dev_err(&intf->dev, "failed to register rc device: %d", ret);
> rc_free_device(rc);
> usb_free_urb(ir->urb);
> --
> 1.9.1
Thanks
Sean
^ permalink raw reply [flat|nested] 3+ messages in thread
* Re: [PATCH] rc: correctly handling failed allocation
2016-02-16 10:54 ` Sean Young
@ 2016-03-03 13:02 ` Mauro Carvalho Chehab
0 siblings, 0 replies; 3+ messages in thread
From: Mauro Carvalho Chehab @ 2016-03-03 13:02 UTC (permalink / raw)
To: Sean Young
Cc: Insu Yun, linux-media, linux-kernel, taesoo, yeongjin.jang, insu,
changwoo
Em Tue, 16 Feb 2016 10:54:54 +0000
Sean Young <sean@mess.org> escreveu:
> On Mon, Feb 15, 2016 at 09:33:11PM -0500, Insu Yun wrote:
> > Since rc_allocate_device() uses kmalloc,
> > it can returns NULL, so need to check,
> > otherwise, NULL derefenrece can be happened.
>
> Thanks for catching that.
>
> > Signed-off-by: Insu Yun <wuninsu@gmail.com>
> > ---
> > drivers/media/rc/igorplugusb.c | 3 +++
> > 1 file changed, 3 insertions(+)
> >
> > diff --git a/drivers/media/rc/igorplugusb.c b/drivers/media/rc/igorplugusb.c
> > index b36e515..df37cd5 100644
> > --- a/drivers/media/rc/igorplugusb.c
> > +++ b/drivers/media/rc/igorplugusb.c
> > @@ -191,6 +191,8 @@ static int igorplugusb_probe(struct usb_interface *intf,
> > usb_make_path(udev, ir->phys, sizeof(ir->phys));
> >
> > rc = rc_allocate_device();
> > + if (!rc)
> > + goto fail;
>
> At this point, ret is not initialized but will be used in the error path.
Also, it should be setting "ret", like like:
if (!rc) {
ret = -ENOMEM;
goto fail;
}
>
> > rc->input_name = DRIVER_DESC;
> > rc->input_phys = ir->phys;
> > usb_to_input_id(udev, &rc->input_id);
> > @@ -213,6 +215,7 @@ static int igorplugusb_probe(struct usb_interface *intf,
> > ir->rc = rc;
> > ret = rc_register_device(rc);
> > if (ret) {
>
> I'm not sure how common it is to goto into another nesting level for an
> error path.
I can't remember a single case where we do that. Putting fail at the
end is indeed the clearer way of handling it.
> Also I just noticed that the code is leaking the timer in
> the error path.
>
> It might be better to put the "fail:" at the end after the last return
> for the successful case, and have a goto to it after both
> rc_allocate_device() and rc_register_device() in case they fail.
>
> > +fail:
> > dev_err(&intf->dev, "failed to register rc device: %d", ret);
> > rc_free_device(rc);
> > usb_free_urb(ir->urb);
> > --
> > 1.9.1
>
> Thanks
> Sean
> --
> To unsubscribe from this list: send the line "unsubscribe linux-media" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at http://vger.kernel.org/majordomo-info.html
--
Thanks,
Mauro
^ permalink raw reply [flat|nested] 3+ messages in thread
end of thread, other threads:[~2016-03-03 13:03 UTC | newest]
Thread overview: 3+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2016-02-16 2:33 [PATCH] rc: correctly handling failed allocation Insu Yun
2016-02-16 10:54 ` Sean Young
2016-03-03 13:02 ` Mauro Carvalho Chehab
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).