Skip to contentSkip to conversation

fujitsu-laptop: remove superfluous NULL pointer checks

PATCH In review Bartlomiej Zolnierkiewicz 2 participants 7 messages Jul 29, 2009 – Jul 31, 2009

Subsystems (3)
ACPI x86 platform drivers Fujitsu laptop extras
lore
Share & tools
lore

Bartlomiej Zolnierkiewicz

author
#0 lore raw

PATCH 1/1 fujitsu-laptop: remove superfluous NULL pointer checks

From: Bartlomiej Zolnierkiewicz <[email protected]>
Subject: [PATCH] fujitsu-laptop: remove superfluous NULL pointer checks

This takes care of the following entries from Dan's list:

drivers/platform/x86/fujitsu-laptop.c +327 set_lcd_level(13) warning: variable derefenced before check 'fujitsu' drivers/platform/x86/fujitsu-laptop.c +358 set_lcd_level_alt(13) warning: variable derefenced before check 'fujitsu'

Reported-by Dan Carpenter <[email protected]>
Cc [email protected]
Cc [email protected]
Cc Julia Lawall <[email protected]>
Signed-off-by Bartlomiej Zolnierkiewicz <[email protected]>
---
 drivers/platform/x86/fujitsu-laptop.c |    6 ------
 1 file changed, 6 deletions(-)
drivers/platform/x86/fujitsu-laptop.c +0 -6
Index: b/drivers/platform/x86/fujitsu-laptop.c
===================================================================
@@ -324,9 +324,6 @@ static int set_lcd_level(int level)
 	if (level < 0 || level >= fujitsu->max_brightness)
 		return -EINVAL;
 
-	if (!fujitsu)
-		return -EINVAL;
-
 	status = acpi_get_handle(fujitsu->acpi_handle, "SBLL", &handle);
 	if (ACPI_FAILURE(status)) {
 		vdbg_printk(FUJLAPTOP_DBG_ERROR, "SBLL not present\n");

@@ -355,9 +352,6 @@ static int set_lcd_level_alt(int level)
 	if (level < 0 || level >= fujitsu->max_brightness)
 		return -EINVAL;
 
-	if (!fujitsu)
-		return -EINVAL;
-
 	status = acpi_get_handle(fujitsu->acpi_handle, "SBL2", &handle);
 	if (ACPI_FAILURE(status)) {
 		vdbg_printk(FUJLAPTOP_DBG_ERROR, "SBL2 not present\n");
#1 lore raw

Hi Bart

Quoted 7 lines

From: Bartlomiej Zolnierkiewicz <[email protected]>
Subject: [PATCH] fujitsu-laptop: remove superfluous NULL pointer checks

This takes care of the following entries from Dan's list:

drivers/platform/x86/fujitsu-laptop.c +327 set_lcd_level(13) warning: variable derefenced before check 'fujitsu' drivers/platform/x86/fujitsu-laptop.c +358 set_lcd_level_alt(13) warning: variable derefenced before check 'fujitsu'

I'd rather keep the test for a non-null fujitsu in there, but obviously it's kind of pointless doing it after the first dereference. Since this fixup overlaps with the one previously discussed with Julia I've taken the liberty of consolidating these - I'll send the result to the list as a separate email.

Regards
jonathan

Quoted 33 lines
Reported-by Dan Carpenter <[email protected]>
Cc [email protected]
Cc [email protected]
Cc Julia Lawall <[email protected]>
Signed-off-by Bartlomiej Zolnierkiewicz <[email protected]>
---
 drivers/platform/x86/fujitsu-laptop.c |    6 ------
 1 file changed, 6 deletions(-)

Index: b/drivers/platform/x86/fujitsu-laptop.c
===================================================================

drivers/platform/x86/fujitsu-laptop.c +0 -6
@@ -324,9 +324,6 @@ static int set_lcd_level(int level)
 	if (level < 0 || level >= fujitsu->max_brightness)
 		return -EINVAL;
 
-	if (!fujitsu)
-		return -EINVAL;
-
 	status = acpi_get_handle(fujitsu->acpi_handle, "SBLL", &handle);
 	if (ACPI_FAILURE(status)) {
 		vdbg_printk(FUJLAPTOP_DBG_ERROR, "SBLL not present\n");

@@ -355,9 +352,6 @@ static int set_lcd_level_alt(int level)
 	if (level < 0 || level >= fujitsu->max_brightness)
 		return -EINVAL;
 
-	if (!fujitsu)
-		return -EINVAL;
-
 	status = acpi_get_handle(fujitsu->acpi_handle, "SBL2", &handle);
 	if (ACPI_FAILURE(status)) {
 		vdbg_printk(FUJLAPTOP_DBG_ERROR, "SBL2 not present\n");
#4 lore raw

Re: [PATCH] fujitsu-laptop: consolidated fixes (NULL pointer checks, [un]registration fixes, etc)

In reply to Bartlomiej Zolnierkiewicz

Hi all

This patch consolidates the two patches posted by Bartlomiej Zolnierkiewicz and the NULL pointer patch which came out of a discussion with Julia Lawall. Some additional NULL pointer check tidy-ups have also been done.

To summarise:

* Take care of the following entries from Dan's list:

drivers/platform/x86/fujitsu-laptop.c +327 set_lcd_level(13) warning:
variable derefenced before check 'fujitsu'
drivers/platform/x86/fujitsu-laptop.c +358 set_lcd_level_alt(13) warning:
variable derefenced before check 'fujitsu'

* Move led_classdev_unregister() calls from fujitsu_cleanup() to acpi_fujitsu_hotkey_remove().

* Fix ordering in fujitsu_cleanup().

* Fix backlight_device_register() failure handling in fujitsu_init().

* Add missing sysfs group removal on failure to fujitsu_init().

* Add input device unregistering on failure to acpi_fujitsu_add() and acpi_fujitsu_hotkey_add().

* Add input device unregistering/freeing to acpi_fujitsu_remove() and acpi_fujitsu_hotkey_remove() (also remove superfluous 'device' and 'acpi_driver_data(device)' checks while at it).

* Do few minor cleanups.

* Fix some additional NULL pointer checks which should occur before the initial dereference.

Patch is relative to linux-acpi-2.6.git head.

Reported-by Dan Carpenter <[email protected]>
Cc [email protected]
Cc [email protected]
Cc Julia Lawall <[email protected]>
Signed-off-by Bartlomiej Zolnierkiewicz <[email protected]>
Signed-off-by Jonathan Woithe <[email protected]>
drivers/platform/x86/fujitsu-laptop.c +58 -53
@@ -70,7 +70,7 @@
 #include <linux/leds.h>
 #endif
 
-#define FUJITSU_DRIVER_VERSION "0.5.0"
+#define FUJITSU_DRIVER_VERSION "0.6.0"
 
 #define FUJITSU_LCD_N_LEVELS 8
 

@@ -321,10 +321,7 @@ static int set_lcd_level(int level)
 	vdbg_printk(FUJLAPTOP_DBG_TRACE, "set lcd level via SBLL [%d]\n",
 		    level);
 
-	if (level < 0 || level >= fujitsu->max_brightness)
-		return -EINVAL;
-
-	if (!fujitsu)
+	if (!fujitsu || level < 0 || level >= fujitsu->max_brightness)
 		return -EINVAL;
 
 	status = acpi_get_handle(fujitsu->acpi_handle, "SBLL", &handle);

@@ -352,10 +349,7 @@ static int set_lcd_level_alt(int level)
 	vdbg_printk(FUJLAPTOP_DBG_TRACE, "set lcd level via SBL2 [%d]\n",
 		    level);
 
-	if (level < 0 || level >= fujitsu->max_brightness)
-		return -EINVAL;
-
-	if (!fujitsu)
+	if (!fujitsu || level < 0 || level >= fujitsu->max_brightness)
 		return -EINVAL;
 
 	status = acpi_get_handle(fujitsu->acpi_handle, "SBL2", &handle);

@@ -697,7 +691,7 @@ static int acpi_fujitsu_add(struct acpi_
 	result = acpi_bus_get_power(fujitsu->acpi_handle, &state);
 	if (result) {
 		printk(KERN_ERR "Error reading power state\n");
-		goto end;
+		goto err_unregister_input_dev;
 	}
 
 	printk(KERN_INFO PREFIX "%s [%s] (%s)\n",

@@ -728,25 +722,31 @@ static int acpi_fujitsu_add(struct acpi_
 
 	return result;
 
-end:
+err_unregister_input_dev:
+	input_unregister_device(input);
 err_free_input_dev:
 	input_free_device(input);
 err_stop:
-
 	return result;
 }
 
 static int acpi_fujitsu_remove(struct acpi_device *device, int type)
 {
 	struct fujitsu_t *fujitsu = NULL;
+	struct input_dev *input = NULL;
 
-	if (!device || !acpi_driver_data(device))
+	if (!device)
 		return -EINVAL;
 
-	fujitsu = acpi_driver_data(device);
-
-	if (!device || !acpi_driver_data(device))
-		return -EINVAL;
+        fujitsu = acpi_driver_data(device);
+        if (!fujitsu)
+        	return -EINVAL;
+        input = fujitsu->input;
+
+        if (input) {
+		input_unregister_device(input);
+		input_free_device(input);
+	}
 
 	fujitsu->acpi_handle = NULL;
 

@@ -871,7 +871,7 @@ static int acpi_fujitsu_hotkey_add(struc
 	result = acpi_bus_get_power(fujitsu_hotkey->acpi_handle, &state);
 	if (result) {
 		printk(KERN_ERR "Error reading power state\n");
-		goto end;
+		goto err_unregister_input_dev;
 	}
 
 	printk(KERN_INFO PREFIX "%s [%s] (%s)\n",

@@ -911,7 +911,7 @@ static int acpi_fujitsu_hotkey_add(struc
 	printk(KERN_INFO "fujitsu-laptop: BTNI: [0x%x]\n",
 		call_fext_func(FUNC_BUTTONS, 0x0, 0x0, 0x0));
 
-	#ifdef CONFIG_LEDS_CLASS
+#ifdef CONFIG_LEDS_CLASS
 	if (call_fext_func(FUNC_LEDS, 0x0, 0x0, 0x0) & LOGOLAMP_POWERON) {
 		result = led_classdev_register(&fujitsu->pf_device->dev,
 						&logolamp_led);

@@ -934,33 +934,50 @@ static int acpi_fujitsu_hotkey_add(struc
 			"LED handler for keyboard lamps, error %i\n", result);
 		}
 	}
-	#endif
+#endif
 
 	return result;
 
-end:
+err_unregister_input_dev:
+	input_unregister_device(input);
 err_free_input_dev:
 	input_free_device(input);
 err_free_fifo:
 	kfifo_free(fujitsu_hotkey->fifo);
 err_stop:
-
 	return result;
 }
 
 static int acpi_fujitsu_hotkey_remove(struct acpi_device *device, int type)
 {
 	struct fujitsu_hotkey_t *fujitsu_hotkey = NULL;
+	struct input_dev *input = NULL;
 
-	if (!device || !acpi_driver_data(device))
+	if (!device)
 		return -EINVAL;
-
 	fujitsu_hotkey = acpi_driver_data(device);
+	if (!fujitsu_hotkey)
+		return -EINVAL;
 
-	fujitsu_hotkey->acpi_handle = NULL;
+	input = fujitsu_hotkey->input;
+
+#ifdef CONFIG_LEDS_CLASS
+	if (fujitsu_hotkey->logolamp_registered)
+		led_classdev_unregister(&logolamp_led);
+
+	if (fujitsu_hotkey->kblamps_registered)
+		led_classdev_unregister(&kblamps_led);
+#endif
+
+	if (input) {
+		input_unregister_device(input);
+		input_free_device(input);
+	}
 
 	kfifo_free(fujitsu_hotkey->fifo);
 
+	fujitsu_hotkey->acpi_handle = NULL;
+
 	return 0;
 }
 

@@ -1130,8 +1147,11 @@ static int __init fujitsu_init(void)
 		fujitsu->bl_device =
 			backlight_device_register("fujitsu-laptop", NULL, NULL,
 						  &fujitsubl_ops);
-		if (IS_ERR(fujitsu->bl_device))
-			return PTR_ERR(fujitsu->bl_device);
+		if (IS_ERR(fujitsu->bl_device)) {
+			ret = PTR_ERR(fujitsu->bl_device);
+			fujitsu->bl_device = NULL;
+			goto fail_sysfs_group;
+		}
 		max_brightness = fujitsu->max_brightness;
 		fujitsu->bl_device->props.max_brightness = max_brightness - 1;
 		fujitsu->bl_device->props.brightness = fujitsu->brightness_level;

@@ -1171,32 +1191,22 @@ static int __init fujitsu_init(void)
 	return 0;
 
 fail_hotkey1:
-
 	kfree(fujitsu_hotkey);
-
 fail_hotkey:
-
 	platform_driver_unregister(&fujitsupf_driver);
-
 fail_backlight:
-
 	if (fujitsu->bl_device)
 		backlight_device_unregister(fujitsu->bl_device);
-
+fail_sysfs_group:
+	sysfs_remove_group(&fujitsu->pf_device->dev.kobj,
+			   &fujitsupf_attribute_group);
 fail_platform_device2:
-
 	platform_device_del(fujitsu->pf_device);
-
 fail_platform_device1:
-
 	platform_device_put(fujitsu->pf_device);
-
 fail_platform_driver:
-
 	acpi_bus_unregister_driver(&acpi_fujitsu_driver);
-
 fail_acpi:
-
 	kfree(fujitsu);
 
 	return ret;

@@ -1204,28 +1214,23 @@ fail_acpi:
 
 static void __exit fujitsu_cleanup(void)
 {
-	#ifdef CONFIG_LEDS_CLASS
-	if (fujitsu_hotkey->logolamp_registered != 0)
-		led_classdev_unregister(&logolamp_led);
+	acpi_bus_unregister_driver(&acpi_fujitsu_hotkey_driver);
 
-	if (fujitsu_hotkey->kblamps_registered != 0)
-		led_classdev_unregister(&kblamps_led);
-	#endif
+	kfree(fujitsu_hotkey);
 
-	sysfs_remove_group(&fujitsu->pf_device->dev.kobj,
-			   &fujitsupf_attribute_group);
-	platform_device_unregister(fujitsu->pf_device);
 	platform_driver_unregister(&fujitsupf_driver);
+
 	if (fujitsu->bl_device)
 		backlight_device_unregister(fujitsu->bl_device);
 
-	acpi_bus_unregister_driver(&acpi_fujitsu_driver);
+	sysfs_remove_group(&fujitsu->pf_device->dev.kobj,
+			   &fujitsupf_attribute_group);
 
-	kfree(fujitsu);
+	platform_device_unregister(fujitsu->pf_device);
 
-	acpi_bus_unregister_driver(&acpi_fujitsu_hotkey_driver);
+	acpi_bus_unregister_driver(&acpi_fujitsu_driver);
 
-	kfree(fujitsu_hotkey);
+	kfree(fujitsu);
 
 	printk(KERN_INFO "fujitsu-laptop: driver unloaded.\n");
 }
author
#5 lore raw

Re: [PATCH] fujitsu-laptop: consolidated fixes (NULL pointer checks, [un]registration fixes, etc)

Quoted 7 lines · On Thursday 30 July 2009 09:55:22 Jonathan Woithe wrote:

Hi all

This patch consolidates the two patches posted by Bartlomiej Zolnierkiewicz and the NULL pointer patch which came out of a discussion with Julia Lawall. Some additional NULL pointer check tidy-ups have also been done.

To summarise:

I think that for the increased readability and to preserve proper credits it would be better to keep separate patches (updating the driver version can be as well handled in an incremental patch).

I would also strongly insist on removing the following needless NULL pointer checks introduced by this version. They may hide real bugs in the ACPI driver code (which should make sure that we always have valid 'device' in the ACPI driver methods and that it doesn't go away while the method executes) or in the fujitsu-laptop itself (which has to always provide valid 'fujitsu_hotkey', 'fujitsu' and 'input' pointers and not free them while there are still some users left).

If this is the case we should fix the underlying problems and not hide them with the "defensive coding". While it could (sometimes) help in avoiding the underlying issue it may also easily result in the whole new bag of issues resulting from introducing the unexpected code paths and system states, i.e. ACPI driver code assumes that the ACPI device is no longer used/referenced by higher-layers (like input layer) and unregistered from the driver after ->remove method finishes -- this assumption will no longer be true in case of premature return caused by 'device == NULL' check if 'device' is NULL (it will never be NULL actually, this is just an example to explain the problem with the "defensive coding" principle).

I understand the sentiment for the "defensive coding" methods (often caused by the real need for them when working on components for the closed code software) but they should be avoided in the open source software.

Thanks.

drivers/platform/x86/fujitsu-laptop.c +30 -12
diff -u b/drivers/platform/x86/fujitsu-laptop.c b/drivers/platform/x86/fujitsu-laptop.c
@@ -70,7 +70,7 @@
 #include <linux/leds.h>
 #endif
 
-#define FUJITSU_DRIVER_VERSION "0.5.0"
+#define FUJITSU_DRIVER_VERSION "0.6.0"
 
 #define FUJITSU_LCD_N_LEVELS 8
 

@@ -321,7 +321,7 @@ 	vdbg_printk(FUJLAPTOP_DBG_TRACE, "set lcd level via SBLL [%d]\n",
 		    level);
 
-	if (level < 0 || level >= fujitsu->max_brightness)
+	if (!fujitsu || level < 0 || level >= fujitsu->max_brightness)
 		return -EINVAL;
 
 	status = acpi_get_handle(fujitsu->acpi_handle, "SBLL", &handle);

@@ -349,7 +349,7 @@
 	vdbg_printk(FUJLAPTOP_DBG_TRACE, "set lcd level via SBL2 [%d]\n",
 		    level);
 
-	if (level < 0 || level >= fujitsu->max_brightness)
+	if (!fujitsu || level < 0 || level >= fujitsu->max_brightness)
 		return -EINVAL;
 
 	status = acpi_get_handle(fujitsu->acpi_handle, "SBL2", &handle);

@@ -732,12 +732,21 @@
 
 static int acpi_fujitsu_remove(struct acpi_device *device, int type)
 {
-	struct fujitsu_t *fujitsu = acpi_driver_data(device);
-	struct input_dev *input = fujitsu->input;
+	struct fujitsu_t *fujitsu = NULL;
+	struct input_dev *input = NULL;
 
-	input_unregister_device(input);
+	if (!device)
+		return -EINVAL;
 
-	input_free_device(input);
+        fujitsu = acpi_driver_data(device);
+        if (!fujitsu)
+        	return -EINVAL;
+        input = fujitsu->input;
+
+        if (input) {
+		input_unregister_device(input);
+		input_free_device(input);
+	}
 
 	fujitsu->acpi_handle = NULL;
 

@@ -941,8 +950,16 @@
 
 static int acpi_fujitsu_hotkey_remove(struct acpi_device *device, int type)
 {
-	struct fujitsu_hotkey_t *fujitsu_hotkey = acpi_driver_data(device);
-	struct input_dev *input = fujitsu_hotkey->input;
+	struct fujitsu_hotkey_t *fujitsu_hotkey = NULL;
+	struct input_dev *input = NULL;
+
+	if (!device)
+		return -EINVAL;
+	fujitsu_hotkey = acpi_driver_data(device);
+	if (!fujitsu_hotkey)
+		return -EINVAL;
+
+	input = fujitsu_hotkey->input;
 
 #ifdef CONFIG_LEDS_CLASS
 	if (fujitsu_hotkey->logolamp_registered)

@@ -952,9 +969,10 @@
 		led_classdev_unregister(&kblamps_led);
 #endif
 
-	input_unregister_device(input);
-
-	input_free_device(input);
+	if (input) {
+		input_unregister_device(input);
+		input_free_device(input);
+	}
 
 	kfifo_free(fujitsu_hotkey->fifo);
author
#2 lore raw
In reply to Jonathan Woithe

Hi,

Quoted 5 lines · On Thursday 30 July 2009 09:43:49 Jonathan Woithe wrote:

Hi Bart

Quoted 3 lines

From: Bartlomiej Zolnierkiewicz <[email protected]>
Subject: [PATCH] fujitsu-laptop: remove superfluous NULL pointer checks

Quoted 8 lines
Quoted 4 lines

This takes care of the following entries from Dan's list:

drivers/platform/x86/fujitsu-laptop.c +327 set_lcd_level(13) warning: variable derefenced before check 'fujitsu' drivers/platform/x86/fujitsu-laptop.c +358 set_lcd_level_alt(13) warning: variable derefenced before check 'fujitsu'

I'd rather keep the test for a non-null fujitsu in there, but obviously it's kind of pointless doing it after the first dereference. Since this fixup overlaps with the one previously discussed with Julia I've taken the liberty

They are on top of Julia's patch (sorry I forgot to mention it)..

#3 lore raw

Hi

Quoted 5 lines
Quoted 5 lines
Quoted 5 lines

From: Bartlomiej Zolnierkiewicz <[email protected]>
Subject: [PATCH] fujitsu-laptop: remove superfluous NULL pointer checks

This takes care of the following entries from Dan's list:

Quoted 8 lines
Quoted 6 lines
Quoted 2 lines

drivers/platform/x86/fujitsu-laptop.c +327 set_lcd_level(13) warning: variable derefenced before check 'fujitsu' drivers/platform/x86/fujitsu-laptop.c +358 set_lcd_level_alt(13) warning: variable derefenced before check 'fujitsu'

I'd rather keep the test for a non-null fujitsu in there, but obviously it's kind of pointless doing it after the first dereference. Since this fixup overlaps with the one previously discussed with Julia I've taken the liberty

They are on top of Julia's patch (sorry I forgot to mention it)..

Ah, right. I was trying to work out where it fitted (that was probably the one preceeding patch I *didn't* end up trying).

Regards
jonathan

#6 lore raw

Re: [PATCH] fujitsu-laptop: consolidated fixes (NULL pointer checks, [un]registration fixes, etc)

In reply to Bartlomiej Zolnierkiewicz

Hi all

Quoted 9 lines
Quoted 5 lines

This patch consolidates the two patches posted by Bartlomiej Zolnierkiewicz and the NULL pointer patch which came out of a discussion with Julia Lawall. Some additional NULL pointer check tidy-ups have also been done.

To summarise:

I think that for the increased readability and to preserve proper credits it would be better to keep separate patches (updating the driver version can be as well handled in an incremental patch).

Ok, leave it with me and I'll re-split along these lines.

Quoted 7 lines

I would also strongly insist on removing the following needless NULL pointer checks introduced by this version. They may hide real bugs in the ACPI driver code (which should make sure that we always have valid 'device' in the ACPI driver methods and that it doesn't go away while the method executes) or in the fujitsu-laptop itself (which has to always provide valid 'fujitsu_hotkey', 'fujitsu' and 'input' pointers and not free them while there are still some users left).

I'll take another look at these. What I was concerned about was whether the hotkey pointer in particular was valid in all cases (since some of the relevant laptops don't in fact have hotkeys) - and I was very short of time yesterday. I've had a very quick look through the code just now and it seems that this may not be a problem after all. In any case I'll confirm this over the next day or so and redo/reisssue the patches accordingly.

Regards
jonathan