Skip to contentSkip to conversation

drivers/scsi/dpt_i2o.c: fix a NULL pointer dereference

PATCH In review Adrian Bunk 3 participants 6 messages Nov 26, 2005 – Nov 28, 2005 SCSI

lore
Share & tools
lore

Adrian Bunk

author
#0 lore raw

PATCH 1/1 drivers/scsi/dpt_i2o.c: fix a NULL pointer dereference

The Coverity checker spotted this obvious NULL pointer dereference.

Signed-off-by Adrian Bunk <[email protected]>
Acked-by Mark Salyzyn <[email protected]>

---

This patch was already sent on:
- 23 Nov 2005
- 21 Nov 2005

 drivers/scsi/dpt_i2o.c |    9 +++++----
 1 file changed, 5 insertions(+), 4 deletions(-)
drivers/scsi/dpt_i2o.c renamed from drivers/scsi/dpt_i2o.c.old +5 -4
@@ -816,7 +816,7 @@
 static void adpt_i2o_sys_shutdown(void)
 {
 	adpt_hba *pHba, *pNext;
-	struct adpt_i2o_post_wait_data *p1, *p2;
+	struct adpt_i2o_post_wait_data *p1, *old;
 
 	 printk(KERN_INFO"Shutting down Adaptec I2O controllers.\n");
 	 printk(KERN_INFO"   This could take a few minutes if there are many devices attached\n");

@@ -830,13 +830,14 @@
 	}
 
 	/* Remove any timedout entries from the wait queue.  */
-	p2 = NULL;
 //	spin_lock_irqsave(&adpt_post_wait_lock, flags);
 	/* Nothing should be outstanding at this point so just
 	 * free them 
 	 */
-	for(p1 = adpt_post_wait_queue; p1; p2 = p1, p1 = p2->next) {
-		kfree(p1);
+	for(p1 = adpt_post_wait_queue; p1;) {
+		old = p1;
+		p1 = p1->next;
+		kfree(old);
 	}
 //	spin_unlock_irqrestore(&adpt_post_wait_lock, flags);
 	adpt_post_wait_queue = NULL;
#1 lore raw
Quoted 1 line · Adrian Bunk

The Coverity checker spotted this obvious NULL pointer dereference.

Hi Adrian,

Could you explain why you remove the adpt_post_wait_lock acquision?

And if it does not belong there, why don't you remove it instead of commeting out?

Quoted 41 lines
Signed-off-by Adrian Bunk <[email protected]>
Acked-by Mark Salyzyn <[email protected]>

---

This patch was already sent on:
- 23 Nov 2005
- 21 Nov 2005

 drivers/scsi/dpt_i2o.c |    9 +++++----
 1 file changed, 5 insertions(+), 4 deletions(-)
drivers/scsi/dpt_i2o.c renamed from drivers/scsi/dpt_i2o.c.old +5 -4
@@ -816,7 +816,7 @@
 static void adpt_i2o_sys_shutdown(void)
 {
 	adpt_hba *pHba, *pNext;
-	struct adpt_i2o_post_wait_data *p1, *p2;
+	struct adpt_i2o_post_wait_data *p1, *old;
 
 	 printk(KERN_INFO"Shutting down Adaptec I2O controllers.\n");
 	 printk(KERN_INFO"   This could take a few minutes if there are many devices attached\n");

@@ -830,13 +830,14 @@
 	}
 
 	/* Remove any timedout entries from the wait queue.  */
-	p2 = NULL;
 //	spin_lock_irqsave(&adpt_post_wait_lock, flags);
 	/* Nothing should be outstanding at this point so just
 	 * free them 
 	 */
-	for(p1 = adpt_post_wait_queue; p1; p2 = p1, p1 = p2->next) {
-		kfree(p1);
+	for(p1 = adpt_post_wait_queue; p1;) {
+		old = p1;
+		p1 = p1->next;
+		kfree(old);
 	}
 //	spin_unlock_irqrestore(&adpt_post_wait_lock, flags);
 	adpt_post_wait_queue = NULL;
author
#2 lore raw
Quoted 4 lines · Marcelo Tosatti
Quoted 1 line · Adrian Bunk

The Coverity checker spotted this obvious NULL pointer dereference.

Hi Adrian,

Hi Marcelo,

Quoted 5 lines

Could you explain why you remove the adpt_post_wait_lock acquision?

And if it does not belong there, why don't you remove it instead of commeting out?
...

my patch does remove the following not required line:

Quoted 1 line

drivers/scsi/dpt_i2o.c line 833

Quoted 1 line
drivers/scsi/dpt_i2o.c renamed from drivers/scsi/dpt_i2o.c.old +0 -1
-	p2 = NULL;

It does not touch the following line that was already commented out:

Quoted 2 lines
Quoted 1 line

// spin_lock_irqsave(&adpt_post_wait_lock, flags);

...

cu
Adrian

Signature · 4 lines
       "Is there not promise of rain?" Ling Tan asked suddenly out
        of the darkness. There had been need of rain for many days.
       "Only a promise," Lao Er said.
                                       Pearl S. Buck - Dragon Seed
#3 lore raw
Quoted 14 lines · Adrian Bunk
Quoted 4 lines · Marcelo Tosatti
Quoted 1 line · Adrian Bunk

The Coverity checker spotted this obvious NULL pointer dereference.

Hi Adrian,

Hi Marcelo,

Quoted 5 lines

Could you explain why you remove the adpt_post_wait_lock acquision?

And if it does not belong there, why don't you remove it instead of commeting out?
...

Quoted 8 lines

my patch does remove the following not required line:

Quoted 1 line

drivers/scsi/dpt_i2o.c line 833

Quoted 1 line
drivers/scsi/dpt_i2o.c renamed from drivers/scsi/dpt_i2o.c.old +0 -1
-	p2 = NULL;

It does not touch the following line that was already commented out:

Quoted 2 lines
Quoted 1 line

// spin_lock_irqsave(&adpt_post_wait_lock, flags);

...

Doh. I should _read_ the patch.

#4 lore raw
In reply to Adrian Bunk
Quoted 1 line · Adrian Bunk

The Coverity checker spotted this obvious NULL pointer dereference.

It's a bit late for this one, since Linus already put it in, but for future reference, could you please try to use proper descriptions. This isn't an "obvious NULL pointer dereference", it's actually a use after free of a data structure.

Thanks,

James

author
#5 lore raw
Quoted 7 lines · James Bottomley
Quoted 1 line · Adrian Bunk

The Coverity checker spotted this obvious NULL pointer dereference.

It's a bit late for this one, since Linus already put it in, but for future reference, could you please try to use proper descriptions. This isn't an "obvious NULL pointer dereference", it's actually a use after free of a data structure.

OK sorry, I'll look better at the descriptions for future patches.

Quoted 3 lines

Thanks,

James

cu
Adrian

Signature · 4 lines
       "Is there not promise of rain?" Ling Tan asked suddenly out
        of the darkness. There had been need of rain for many days.
       "Only a promise," Lao Er said.
                                       Pearl S. Buck - Dragon Seed