* [patch] [media] em28xx-input: NULL dereference on error
@ 2014-09-25 11:39 Dan Carpenter
2014-09-25 14:08 ` Frank Schäfer
0 siblings, 1 reply; 6+ messages in thread
From: Dan Carpenter @ 2014-09-25 11:39 UTC (permalink / raw)
To: Mauro Carvalho Chehab, Frank Schaefer; +Cc: linux-media, kernel-janitors
We call "kfree(ir->i2c_client);" in the error handling and that doesn't
work if "ir" is NULL.
Fixes: 78e719a5f30b ('[media] em28xx-input: i2c IR decoders: improve i2c_client handling')
Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com>
diff --git a/drivers/media/usb/em28xx/em28xx-input.c b/drivers/media/usb/em28xx/em28xx-input.c
index 581f6da..23f8f6a 100644
--- a/drivers/media/usb/em28xx/em28xx-input.c
+++ b/drivers/media/usb/em28xx/em28xx-input.c
@@ -712,8 +712,10 @@ static int em28xx_ir_init(struct em28xx *dev)
em28xx_info("Registering input extension\n");
ir = kzalloc(sizeof(*ir), GFP_KERNEL);
+ if (!ir)
+ return -ENOMEM;
rc = rc_allocate_device();
- if (!ir || !rc)
+ if (!rc)
goto error;
/* record handles to ourself */
^ permalink raw reply related [flat|nested] 6+ messages in thread* Re: [patch] [media] em28xx-input: NULL dereference on error
2014-09-25 11:39 [patch] [media] em28xx-input: NULL dereference on error Dan Carpenter
@ 2014-09-25 14:08 ` Frank Schäfer
2014-09-25 14:49 ` Dan Carpenter
2014-09-25 15:37 ` Julia Lawall
0 siblings, 2 replies; 6+ messages in thread
From: Frank Schäfer @ 2014-09-25 14:08 UTC (permalink / raw)
To: Dan Carpenter, Mauro Carvalho Chehab; +Cc: linux-media, kernel-janitors
Hi Dan,
Am 25.09.2014 um 13:39 schrieb Dan Carpenter:
> We call "kfree(ir->i2c_client);" in the error handling and that doesn't
> work if "ir" is NULL.
>
> Fixes: 78e719a5f30b ('[media] em28xx-input: i2c IR decoders: improve i2c_client handling')
> Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com>
>
> diff --git a/drivers/media/usb/em28xx/em28xx-input.c b/drivers/media/usb/em28xx/em28xx-input.c
> index 581f6da..23f8f6a 100644
> --- a/drivers/media/usb/em28xx/em28xx-input.c
> +++ b/drivers/media/usb/em28xx/em28xx-input.c
> @@ -712,8 +712,10 @@ static int em28xx_ir_init(struct em28xx *dev)
> em28xx_info("Registering input extension\n");
>
> ir = kzalloc(sizeof(*ir), GFP_KERNEL);
> + if (!ir)
> + return -ENOMEM;
> rc = rc_allocate_device();
> - if (!ir || !rc)
> + if (!rc)
> goto error;
>
> /* record handles to ourself */
I would prefer to fix it where the actual problem is located.
Can you send an updated version that changes the code to do
...
error:
if (ir)
kfree(ir->i2c_client);
...
This makes the code less prone to future error handling changes.
Thanks !
Regards,
Frank
^ permalink raw reply [flat|nested] 6+ messages in thread* Re: [patch] [media] em28xx-input: NULL dereference on error
2014-09-25 14:08 ` Frank Schäfer
@ 2014-09-25 14:49 ` Dan Carpenter
2014-09-25 17:50 ` Frank Schäfer
2014-09-25 15:37 ` Julia Lawall
1 sibling, 1 reply; 6+ messages in thread
From: Dan Carpenter @ 2014-09-25 14:49 UTC (permalink / raw)
To: Frank Schäfer; +Cc: Mauro Carvalho Chehab, linux-media, kernel-janitors
On Thu, Sep 25, 2014 at 04:08:31PM +0200, Frank Schäfer wrote:
> > ir = kzalloc(sizeof(*ir), GFP_KERNEL);
> > + if (!ir)
> > + return -ENOMEM;
> > rc = rc_allocate_device();
> > - if (!ir || !rc)
> > + if (!rc)
> > goto error;
> >
> > /* record handles to ourself */
> I would prefer to fix it where the actual problem is located.
> Can you send an updated version that changes the code to do
>
> ...
> error:
> if (ir)
> kfree(ir->i2c_client);
> ...
>
> This makes the code less prone to future error handling changes.
This kind of bug is called a "One Err Bug" because they are part of
an anti-pattern of bad error handling where there is only one label. It
was ok at the time it was written but it was fragile and broke when the
code changed.
One Err Bugs are very common kind of bug. I just reported a similar bug
this morning. https://lkml.org/lkml/2014/9/25/91 In that case we freed
some sysfs files which were not allocated.
My view is that error handling code should not have if statements unless
there is an if statement in the allocation code. This is way more
readable.
Another way that people deal with these kinds of errors if they don't
like to return directly is they add an "out:" label.
out:
return ret;
I hate "out" labels for how vague the name is but I also hate do-nothing
gotos generally. When you're reading the code you assume that the goto
does something but the name gives you no clue what it does so you have
to interrupt what you are doing and scroll down to the bottom of the
function and it doesn't do anything. It just returns. By this point
you have forgotten where you were but it was somewhere reading in the
middle of the function.
Terrible terrible terrible. Etc. You have touched a sore spot and
triggered a rant. :P
regards,
dan carpenter
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [patch] [media] em28xx-input: NULL dereference on error
2014-09-25 14:49 ` Dan Carpenter
@ 2014-09-25 17:50 ` Frank Schäfer
0 siblings, 0 replies; 6+ messages in thread
From: Frank Schäfer @ 2014-09-25 17:50 UTC (permalink / raw)
To: Dan Carpenter; +Cc: Mauro Carvalho Chehab, linux-media, kernel-janitors
Am 25.09.2014 um 16:49 schrieb Dan Carpenter:
> On Thu, Sep 25, 2014 at 04:08:31PM +0200, Frank Schäfer wrote:
>>> ir = kzalloc(sizeof(*ir), GFP_KERNEL);
>>> + if (!ir)
>>> + return -ENOMEM;
>>> rc = rc_allocate_device();
>>> - if (!ir || !rc)
>>> + if (!rc)
>>> goto error;
>>>
>>> /* record handles to ourself */
>> I would prefer to fix it where the actual problem is located.
>> Can you send an updated version that changes the code to do
>>
>> ...
>> error:
>> if (ir)
>> kfree(ir->i2c_client);
>> ...
>>
>> This makes the code less prone to future error handling changes.
> This kind of bug is called a "One Err Bug" because they are part of
> an anti-pattern of bad error handling where there is only one label. It
> was ok at the time it was written but it was fragile and broke when the
> code changed.
>
> One Err Bugs are very common kind of bug. I just reported a similar bug
> this morning. https://lkml.org/lkml/2014/9/25/91 In that case we freed
> some sysfs files which were not allocated.
>
> My view is that error handling code should not have if statements unless
> there is an if statement in the allocation code. This is way more
> readable.
>
> Another way that people deal with these kinds of errors if they don't
> like to return directly is they add an "out:" label.
>
> out:
> return ret;
>
> I hate "out" labels for how vague the name is but I also hate do-nothing
> gotos generally. When you're reading the code you assume that the goto
> does something but the name gives you no clue what it does so you have
> to interrupt what you are doing and scroll down to the bottom of the
> function and it doesn't do anything. It just returns. By this point
> you have forgotten where you were but it was somewhere reading in the
> middle of the function.
Dan,
I 100% agree with everything you are saying here about lables, error
handling etc.
And your fix is of course 100% valid.
I would have a much better feeling if we add a NULL-pointer check before
the kfree, because it makes things more difficult to break in the future.
I've seen that happen too often.
Anyway, go ahead with your patch. No need to waste more time.
Acked-by: Frank Schäfer <fschaefer.oss@googlemail.com>
Thanks for pointing this out, em28xx can't get enough attention.
Regards,
Frank
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [patch] [media] em28xx-input: NULL dereference on error
2014-09-25 14:08 ` Frank Schäfer
2014-09-25 14:49 ` Dan Carpenter
@ 2014-09-25 15:37 ` Julia Lawall
2014-09-25 16:28 ` Mauro Carvalho Chehab
1 sibling, 1 reply; 6+ messages in thread
From: Julia Lawall @ 2014-09-25 15:37 UTC (permalink / raw)
To: Frank Schäfer
Cc: Dan Carpenter, Mauro Carvalho Chehab, linux-media,
kernel-janitors
[-- Attachment #1: Type: TEXT/PLAIN, Size: 1562 bytes --]
On Thu, 25 Sep 2014, Frank Schäfer wrote:
> Hi Dan,
>
> Am 25.09.2014 um 13:39 schrieb Dan Carpenter:
> > We call "kfree(ir->i2c_client);" in the error handling and that doesn't
> > work if "ir" is NULL.
> >
> > Fixes: 78e719a5f30b ('[media] em28xx-input: i2c IR decoders: improve i2c_client handling')
> > Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com>
> >
> > diff --git a/drivers/media/usb/em28xx/em28xx-input.c b/drivers/media/usb/em28xx/em28xx-input.c
> > index 581f6da..23f8f6a 100644
> > --- a/drivers/media/usb/em28xx/em28xx-input.c
> > +++ b/drivers/media/usb/em28xx/em28xx-input.c
> > @@ -712,8 +712,10 @@ static int em28xx_ir_init(struct em28xx *dev)
> > em28xx_info("Registering input extension\n");
> >
> > ir = kzalloc(sizeof(*ir), GFP_KERNEL);
> > + if (!ir)
> > + return -ENOMEM;
> > rc = rc_allocate_device();
> > - if (!ir || !rc)
> > + if (!rc)
> > goto error;
I have never understood this kind of code. If the kmalloc fails, why not
give up immediately (as in Dan's patch)?
julia
> > /* record handles to ourself */
> I would prefer to fix it where the actual problem is located.
> Can you send an updated version that changes the code to do
>
> ...
> error:
> if (ir)
> kfree(ir->i2c_client);
> ...
>
> This makes the code less prone to future error handling changes.
>
> Thanks !
>
> Regards,
> Frank
>
> --
> To unsubscribe from this list: send the line "unsubscribe kernel-janitors" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at http://vger.kernel.org/majordomo-info.html
>
^ permalink raw reply [flat|nested] 6+ messages in thread* Re: [patch] [media] em28xx-input: NULL dereference on error
2014-09-25 15:37 ` Julia Lawall
@ 2014-09-25 16:28 ` Mauro Carvalho Chehab
0 siblings, 0 replies; 6+ messages in thread
From: Mauro Carvalho Chehab @ 2014-09-25 16:28 UTC (permalink / raw)
To: Julia Lawall
Cc: Frank Schäfer, Dan Carpenter, linux-media, kernel-janitors
Em Thu, 25 Sep 2014 17:37:46 +0200
Julia Lawall <julia.lawall@lip6.fr> escreveu:
> On Thu, 25 Sep 2014, Frank Schäfer wrote:
>
> > Hi Dan,
> >
> > Am 25.09.2014 um 13:39 schrieb Dan Carpenter:
> > > We call "kfree(ir->i2c_client);" in the error handling and that doesn't
> > > work if "ir" is NULL.
> > >
> > > Fixes: 78e719a5f30b ('[media] em28xx-input: i2c IR decoders: improve i2c_client handling')
> > > Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com>
> > >
> > > diff --git a/drivers/media/usb/em28xx/em28xx-input.c b/drivers/media/usb/em28xx/em28xx-input.c
> > > index 581f6da..23f8f6a 100644
> > > --- a/drivers/media/usb/em28xx/em28xx-input.c
> > > +++ b/drivers/media/usb/em28xx/em28xx-input.c
> > > @@ -712,8 +712,10 @@ static int em28xx_ir_init(struct em28xx *dev)
> > > em28xx_info("Registering input extension\n");
> > >
> > > ir = kzalloc(sizeof(*ir), GFP_KERNEL);
> > > + if (!ir)
> > > + return -ENOMEM;
> > > rc = rc_allocate_device();
> > > - if (!ir || !rc)
> > > + if (!rc)
> > > goto error;
>
> I have never understood this kind of code. If the kmalloc fails, why not
> give up immediately (as in Dan's patch)?
I agree. In this specific place, it can just return an error, as there's
nothing yet used, just like the previous clauses.
Regards,
Mauro
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2014-09-25 17:48 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2014-09-25 11:39 [patch] [media] em28xx-input: NULL dereference on error Dan Carpenter
2014-09-25 14:08 ` Frank Schäfer
2014-09-25 14:49 ` Dan Carpenter
2014-09-25 17:50 ` Frank Schäfer
2014-09-25 15:37 ` Julia Lawall
2014-09-25 16:28 ` 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