[PATCH 2/6] drivers/md: remove null pointer dereference

[PATCH 2/6] drivers/md: remove null pointer dereference

[Julia Lawall]

From: Julia Lawall <julia@diku.dk>

If pgpath->pg->ps.type is NULL, it is not possible to access its name
field.  So I have simply modified the error message to drop the printing of
the name field.


This problem was found using the following semantic match
(http://www.emn.fr/x-info/coccinelle/)

// <smpl>
@@
expression E, E1;
identifier f;
statement S1,S2,S3;
@@

* if (E == NULL)
{
  ... when != if (E == NULL) S1 else S2
      when != E = E1
* E->f
  ... when any
  return ...;
}
else S3
// </smpl>

Signed-off-by: Julia Lawall <julia@diku.dk>

---

diff -u -p a/drivers/md/dm-mpath.c b/drivers/md/dm-mpath.c
--- a/drivers/md/dm-mpath.c	2008-04-16 13:27:57.000000000 +0200
+++ b/drivers/md/dm-mpath.c	2008-05-12 09:19:35.000000000 +0200
@@ -884,8 +884,7 @@ static int reinstate_path(struct pgpath 
 		goto out;
 
 	if (!pgpath->pg->ps.type) {
-		DMWARN("Reinstate path not supported by path selector %s",
-		       pgpath->pg->ps.type->name);
+		DMWARN("Reinstate path not supported by path selector");
 		r = -EINVAL;
 		goto out;
 	}

Re: [dm-devel] [PATCH 2/6] drivers/md: remove null pointer dereference

[Alasdair G Kergon]

On Mon, May 12, 2008 at 03:37:31PM +0200, Julia Lawall wrote:
> If pgpath->pg->ps.type is NULL, it is not possible to access its name
> field.  So I have simply modified the error message to drop the printing of
> the name field.
> 
> This problem was found using the following semantic match
> (http://www.emn.fr/x-info/coccinelle/)
 
> --- a/drivers/md/dm-mpath.c	2008-04-16 13:27:57.000000000 +0200
> +++ b/drivers/md/dm-mpath.c	2008-05-12 09:19:35.000000000 +0200
> @@ -884,8 +884,7 @@ static int reinstate_path(struct pgpath 
>  		goto out;
>  
>  	if (!pgpath->pg->ps.type) {
> -		DMWARN("Reinstate path not supported by path selector %s",
> -		       pgpath->pg->ps.type->name);
> +		DMWARN("Reinstate path not supported by path selector");
>  		r = -EINVAL;
>  		goto out;
>  	}
 
Thanks for reporting this.

A more-sophisticated checker might discover that the test can never fail
- see parse_path_selector() - and so the real problem here is that it is
the wrong test.

The next line is:
	r = pgpath->pg->ps.type->reinstate_path(&pgpath->pg->ps, &pgpath->path);
and the error message makes it clear that the intent was to ensure that
the reinstate_path method exists before attempting to use it.

IOW
	if (!pgpath->pg->ps.type->reinstate_path) {

Alasdair
-- 
agk@redhat.com

Re: [dm-devel] [PATCH 2/6] drivers/md: remove null pointer dereference

[Julia Lawall]

On Wed, 2 Jul 2008, Alasdair G Kergon wrote:

> On Mon, May 12, 2008 at 03:37:31PM +0200, Julia Lawall wrote:
> > If pgpath->pg->ps.type is NULL, it is not possible to access its name
> > field.  So I have simply modified the error message to drop the printing of
> > the name field.
> > 
> > This problem was found using the following semantic match
> > (http://www.emn.fr/x-info/coccinelle/)
>  
> > --- a/drivers/md/dm-mpath.c	2008-04-16 13:27:57.000000000 +0200
> > +++ b/drivers/md/dm-mpath.c	2008-05-12 09:19:35.000000000 +0200
> > @@ -884,8 +884,7 @@ static int reinstate_path(struct pgpath 
> >  		goto out;
> >  
> >  	if (!pgpath->pg->ps.type) {
> > -		DMWARN("Reinstate path not supported by path selector %s",
> > -		       pgpath->pg->ps.type->name);
> > +		DMWARN("Reinstate path not supported by path selector");
> >  		r = -EINVAL;
> >  		goto out;
> >  	}
>  
> Thanks for reporting this.
> 
> A more-sophisticated checker might discover that the test can never fail
> - see parse_path_selector() - and so the real problem here is that it is
> the wrong test.
> 
> The next line is:
> 	r = pgpath->pg->ps.type->reinstate_path(&pgpath->pg->ps, &pgpath->path);
> and the error message makes it clear that the intent was to ensure that
> the reinstate_path method exists before attempting to use it.
> 
> IOW
> 	if (!pgpath->pg->ps.type->reinstate_path) {

Thanks for the suggestions.

In looking at it a little bit, it seems that ps.type is initialized in the
function stored in the ctr field of the target_type structure and this
function is called in the function stored in the message field of the same
structure.  The function in the message structure seems to be only called
from the function target_message in dm-ioctl.c, but I don't see the
relation to an invocation of the ctr field.  Is that guaranteed to be
invoked earlier?

julia

Re: [dm-devel] [PATCH 2/6] drivers/md: remove null pointer dereference

[Alasdair G Kergon]

On Wed, Jul 02, 2008 at 01:51:41PM +0200, Julia Lawall wrote:
> In looking at it a little bit, it seems that ps.type is initialized in the
> function stored in the ctr field of the target_type structure and this
> function is called in the function stored in the message field of the same
> structure.  The function in the message structure seems to be only called
> from the function target_message in dm-ioctl.c, but I don't see the
> relation to an invocation of the ctr field.  Is that guaranteed to be
> invoked earlier?
 
ctr is short for constructor (perhaps we should rename these functions one day)
and it has to run first to set up the data structures which a subsequent
target_message may reference.

Alasdair
-- 
agk@redhat.com