[PATCHv2 13/13] xfree86: fix memory leak in xf86LoadModules

[Mark Kettenis]

> Date: Mon, 28 Mar 2011 10:48:06 -0700
> From: Alan Coopersmith <alan.coopersmith at oracle.com>
> 
> On 03/28/11 06:14 AM, Tiago Vignatti wrote:
> > Signed-off-by: Tiago Vignatti <tiago.vignatti at nokia.com>
> > ---
> >  hw/xfree86/common/xf86Init.c |    1 +
> >  1 files changed, 1 insertions(+), 0 deletions(-)
> > 
> > diff --git a/hw/xfree86/common/xf86Init.c b/hw/xfree86/common/xf86Init.c
> > index e664ce4..d81303e 100644
> > --- a/hw/xfree86/common/xf86Init.c
> > +++ b/hw/xfree86/common/xf86Init.c
> > @@ -1433,6 +1433,7 @@ xf86LoadModules(char **list, pointer *optlist)
> >  	}
> >  	free(name);
> >      }
> > +    free(name);
> >      return !failed;
> >  }
> >  
> 
> NAK - this is still obviously a double-free, which is a security vulnerability
> on some platforms ( http://cwe.mitre.org/data/definitions/415.html ), nor
> does it solve the leak in all situations.
> 
> The leak is due to this code:
> 
>         /* Skip empty names */
>         if (name == NULL || *name == '\0')
>             continue;
> 
> but since that's continue instead of break, it jumps to the next iteration
> of the loop, not out of the loop, so if that happens in the middle of the
> list, the name variable will still be overwritten without being freed.
> 
> A better fix would seem to be:
> 
>         /* Skip empty names */
>         if (name == NULL || *name == '\0') {
> 	    free(name);
>             continue;
> 	}
> 
> (since free(NULL) is guaranteed safe/no-op by ANSI C89 & later), or
> if you want to be really picky:
> 
>         /* Skip empty names */
>         if (name == NULL)
>             continue;
>         if (*name == '\0') {
> 	    free(name);
>             continue;
> 	}

The last mainstream OS with a free(3) that didn't accept NULL was
SunOS 4.x, so I think your first version is perfectly fine.