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

[Alan Coopersmith]

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;
	}

-- 
	-Alan Coopersmith-        alan.coopersmith at oracle.com
	 Oracle Solaris Platform Engineering: X Window System