> Date: Mon, 28 Mar 2011 10:48:06 -0700 > From: Alan Coopersmith <[email protected]> > > On 03/28/11 06:14 AM, Tiago Vignatti wrote: > > Signed-off-by: Tiago Vignatti <[email protected]> > > --- > > 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. _______________________________________________ [email protected]: X.Org development Archives: http://lists.x.org/archives/xorg-devel Info: http://lists.x.org/mailman/listinfo/xorg-devel
