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(-)
--- linux-2.6.15-rc1-mm2-full/drivers/scsi/dpt_i2o.c.old 2005-11-20 22:13:37.000000000 +0100
+++ linux-2.6.15-rc1-mm2-full/drivers/scsi/dpt_i2o.c 2005-11-20 22:16:57.000000000 +0100
@@ -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;
On Sun, Nov 27, 2005 at 12:36:37AM +0100, Adrian Bunk wrote:
> 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?
> 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(-)
>
> --- linux-2.6.15-rc1-mm2-full/drivers/scsi/dpt_i2o.c.old 2005-11-20 22:13:37.000000000 +0100
> +++ linux-2.6.15-rc1-mm2-full/drivers/scsi/dpt_i2o.c 2005-11-20 22:16:57.000000000 +0100
> @@ -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;
On Sun, Nov 27, 2005 at 10:47:38AM -0200, Marcelo Tosatti wrote:
> On Sun, Nov 27, 2005 at 12:36:37AM +0100, Adrian Bunk wrote:
> > The Coverity checker spotted this obvious NULL pointer dereference.
>
> Hi Adrian,
Hi Marcelo,
> 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:
> > - p2 = NULL;
It does not touch the following line that was already commented out:
> > // spin_lock_irqsave(&adpt_post_wait_lock, flags);
>...
cu
Adrian
--
"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
On Sun, Nov 27, 2005 at 07:52:52PM +0100, Adrian Bunk wrote:
> On Sun, Nov 27, 2005 at 10:47:38AM -0200, Marcelo Tosatti wrote:
> > On Sun, Nov 27, 2005 at 12:36:37AM +0100, Adrian Bunk wrote:
> > > The Coverity checker spotted this obvious NULL pointer dereference.
> >
> > Hi Adrian,
>
> Hi Marcelo,
>
> > 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:
>
> > > - p2 = NULL;
>
> It does not touch the following line that was already commented out:
>
> > > // spin_lock_irqsave(&adpt_post_wait_lock, flags);
> >...
Doh. I should _read_ the patch.
On Sun, 2005-11-27 at 00:36 +0100, Adrian Bunk wrote:
> 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
On Mon, Nov 28, 2005 at 12:37:11PM -0600, James Bottomley wrote:
> On Sun, 2005-11-27 at 00:36 +0100, Adrian Bunk wrote:
> > 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.
> Thanks,
>
> James
cu
Adrian
--
"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