Skip to contentSkip to conversation

em28xx-input: NULL dereference on error

PATCH In review Dan Carpenter 4 participants 6 messages Sep 25, 2014

Subsystems (3)
Media (V4L/DVB) Kernel janitors EM28XX VIDEO4LINUX driver
lore
Share & tools
lore

Dan Carpenter

author
#0 lore raw

PATCH 1/1 em28xx-input: NULL dereference on error

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 <[email protected]>
drivers/media/usb/em28xx/em28xx-input.c +3 -1
index 581f6da..23f8f6a 100644
@@ -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 */
#1 lore raw

Hi Dan,

Am 25.09.2014 um 13:39 schrieb Dan Carpenter:

Quoted 12 lines

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 <[email protected]>
drivers/media/usb/em28xx/em28xx-input.c +0 -0
index 581f6da..23f8f6a 100644
@@ -712,8 +712,10 @@ static int em28xx_ir_init(struct em28xx *dev)
 	em28xx_info("Registering input extension\n");

drivers/media/usb/em28xx/em28xx-input.c static int em28xx_ir_init(struct em28xx *dev) · line 721

Quoted 10 lines
drivers/media/usb/em28xx/em28xx-input.c +3 -1
 
 	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

author
#2 lore raw
Quoted 12 lines · Frank Schäfer

drivers/media/usb/em28xx/em28xx-input.c static int em28xx_ir_init(struct em28xx *dev) · line 721

Quoted 9 lines
drivers/media/usb/em28xx/em28xx-input.c +3 -1
 	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

Quoted 7 lines

...
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

--
To unsubscribe from this list: send the line "unsubscribe kernel-janitors" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html

Acked-by
#3 lore raw

Am 25.09.2014 um 16:49 schrieb Dan Carpenter:

Quoted 39 lines · Frank Schäfer
Quoted 12 lines

drivers/media/usb/em28xx/em28xx-input.c static int em28xx_ir_init(struct em28xx *dev) · line 721

Quoted 9 lines
drivers/media/usb/em28xx/em28xx-input.c +3 -1
 	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

Quoted 7 lines

...
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;
Quoted 7 lines

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 <[email protected]>

Thanks for pointing this out, em28xx can't get enough attention.

Regards,
Frank

#4 lore raw
In reply to Frank Schäfer
Quoted 16 lines · Frank Schäfer

Hi Dan,

Am 25.09.2014 um 13:39 schrieb Dan Carpenter:

Quoted 13 lines

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 <[email protected]>
drivers/media/usb/em28xx/em28xx-input.c +0 -0
index 581f6da..23f8f6a 100644
@@ -712,8 +712,10 @@ static int em28xx_ir_init(struct em28xx *dev)
 	em28xx_info("Registering input extension\n");
Quoted 7 lines

drivers/media/usb/em28xx/em28xx-input.c static int em28xx_ir_init(struct em28xx *dev) · line 719

Quoted 7 lines
drivers/media/usb/em28xx/em28xx-input.c +3 -1
 	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

Quoted 22 lines

drivers/media/usb/em28xx/em28xx-input.c static int em28xx_ir_init(struct em28xx *dev) · line 721

Quoted 1 line
drivers/media/usb/em28xx/em28xx-input.c +0 -0
 	/* 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 [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html

#5 lore raw

Em Thu, 25 Sep 2014 17:37:46 +0200
Julia Lawall <[email protected]> escreveu:

Quoted 18 lines · Frank Schäfer
Quoted 17 lines

Hi Dan,

Am 25.09.2014 um 13:39 schrieb Dan Carpenter:

Quoted 13 lines

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 <[email protected]>
drivers/media/usb/em28xx/em28xx-input.c +0 -0
index 581f6da..23f8f6a 100644
@@ -712,8 +712,10 @@ static int em28xx_ir_init(struct em28xx *dev)
 	em28xx_info("Registering input extension\n");
Quoted 10 lines
Quoted 7 lines

drivers/media/usb/em28xx/em28xx-input.c static int em28xx_ir_init(struct em28xx *dev) · line 719

Quoted 7 lines
drivers/media/usb/em28xx/em28xx-input.c +3 -1
 	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
--
To unsubscribe from this list: send the line "unsubscribe kernel-janitors" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html