[PATCH 2/5] Correctly free config file names

[Dan Nicholson]

On Nov 1, 2011 6:13 AM, <przanoni at gmail.com> wrote:
>
> From: Paulo Zanoni <paulo.r.zanoni at intel.com>
>
> We call xf86penConfigDirFiles twice, so we overwrite the configDirPath
> variable, losing the pointer. If we move the pointer management to the
> upper layer (the function callers), they will be able to call these
> functions as many times as they want, but they'll have to free those
> returned values.
>
> 4,097 bytes in 1 blocks are definitely lost in loss record 625 of 632
>   at 0x4C2779D: malloc (in vgpreload_memcheck-amd64-linux.so)
>   by 0x4D7899: DoSubstitution (scan.c:615)
>   by 0x4D87B0: OpenConfigDir (scan.c:845)
>   by 0x4D8A2D: xf86openConfigDirFiles (scan.c:955)
>   by 0x49031F: xf86HandleConfigFile (xf86Config.c:2327)
>   by 0x49A9BF: InitOutput (xf86Init.c:365)
>   by 0x425A7A: main (main.c:204)
>
> Signed-off-by: Paulo Zanoni <paulo.r.zanoni at intel.com>
> ---
>  hw/xfree86/common/xf86Config.c |    6 +++++-
>  hw/xfree86/parser/scan.c       |   22 ++++++++--------------
>  hw/xfree86/parser/xf86Parser.h |    8 ++++----
>  3 files changed, 17 insertions(+), 19 deletions(-)
>
> diff --git a/hw/xfree86/common/xf86Config.c b/hw/xfree86/common/xf86Config.c
> index cb4be42..5ab3e67 100644
> --- a/hw/xfree86/common/xf86Config.c
> +++ b/hw/xfree86/common/xf86Config.c
> @@ -2301,7 +2301,7 @@ checkInput(serverLayoutPtr layout, Bool implicit_layout) {
>  ConfigStatus
>  xf86HandleConfigFile(Bool autoconfig)
>  {
> -    const char *filename, *dirname, *sysdirname;
> +    char *filename, *dirname, *sysdirname;
>     char *filesearch, *dirsearch;
>     MessageType filefrom = X_DEFAULT;
>     MessageType dirfrom = X_DEFAULT;
> @@ -2353,6 +2353,10 @@ xf86HandleConfigFile(Bool autoconfig)
>            return CONFIG_NOFILE;
>     }
>
> +    free(filename);
> +    free(dirname);
> +    free(sysdirname);
> +
>     if ((xf86configptr = xf86readConfigFile ()) == NULL) {
>        xf86Msg(X_ERROR, "Problem parsing the config file\n");
>        return CONFIG_PARSE_ERROR;
> diff --git a/hw/xfree86/parser/scan.c b/hw/xfree86/parser/scan.c
> index 99b3257..668237b 100644
> --- a/hw/xfree86/parser/scan.c
> +++ b/hw/xfree86/parser/scan.c
> @@ -101,8 +101,6 @@ static int builtinIndex = 0;
>  static int configPos = 0;              /* current readers position */
>  static int configLineNo = 0;   /* linenumber */
>  static char *configBuf, *configRBuf;   /* buffer for lines */
> -static char *configPath;               /* path to config file */
> -static char *configDirPath;            /* path to config dir */
>  static char *configSection = NULL;     /* name of current section being parsed */
>  static int numFiles = 0;               /* number of config files */
>  static int curFileIndex = 0;           /* index of current config file */
> @@ -892,7 +890,8 @@ xf86initConfigFiles(void)
>  * of the located files.
>  *
>  * The return value is a pointer to the actual name of the file that was
> - * opened.  When no file is found, the return value is NULL.
> + * opened.  When no file is found, the return value is NULL. The caller should
> + * free() the returned value.
>  *
>  * The escape sequences allowed in the search path are defined above.
>  *
> @@ -914,7 +913,7 @@ xf86initConfigFiles(void)
>                                                        "%P/lib/X11/%X"
>  #endif
>
> -const char *
> +char *
>  xf86openConfigFile(const char *path, const char *cmdline, const char *projroot)
>  {
>        if (!path || !path[0])
> @@ -923,8 +922,7 @@ xf86openConfigFile(const char *path, const char *cmdline, const char *projroot)
>                projroot = PROJECTROOT;
>
>        /* Search for a config file */
> -       configPath = OpenConfigFile(path, cmdline, projroot, XCONFIGFILE);
> -       return configPath;
> +       return OpenConfigFile(path, cmdline, projroot, XCONFIGFILE);
>  }
>
>  /*
> @@ -937,12 +935,13 @@ xf86openConfigFile(const char *path, const char *cmdline, const char *projroot)
>  * fails if it is not found.
>  *
>  * The return value is a pointer to the actual name of the direcoty that was
> - * opened.  When no directory is found, the return value is NULL.
> + * opened.  When no directory is found, the return value is NULL. The caller
> + * should free() the returned value.
>  *
>  * The escape sequences allowed in the search path are defined above.
>  *
>  */
> -const char *
> +char *
>  xf86openConfigDirFiles(const char *path, const char *cmdline,
>                       const char *projroot)
>  {
> @@ -952,8 +951,7 @@ xf86openConfigDirFiles(const char *path, const char *cmdline,
>                projroot = PROJECTROOT;
>
>        /* Search for the multiconf directory */
> -       configDirPath = OpenConfigDir(path, cmdline, projroot, XCONFIGDIR);
> -       return configDirPath;
> +       return OpenConfigDir(path, cmdline, projroot, XCONFIGDIR);
>  }
>
>  void
> @@ -961,10 +959,6 @@ xf86closeConfigFile (void)
>  {
>        int i;
>
> -       free (configPath);
> -       configPath = NULL;
> -       free (configDirPath);
> -       configDirPath = NULL;
>        free (configRBuf);
>        configRBuf = NULL;
>        free (configBuf);
> diff --git a/hw/xfree86/parser/xf86Parser.h b/hw/xfree86/parser/xf86Parser.h
> index a8785c5..c31fdc4 100644
> --- a/hw/xfree86/parser/xf86Parser.h
> +++ b/hw/xfree86/parser/xf86Parser.h
> @@ -487,10 +487,10 @@ xf86ConfigSymTabRec, *xf86ConfigSymTabPtr;
>  * prototypes for public functions
>  */
>  extern void xf86initConfigFiles(void);
> -extern const char *xf86openConfigFile(const char *path, const char *cmdline,
> -                                     const char *projroot);
> -extern const char *xf86openConfigDirFiles(const char *path, const char *cmdline,
> -                                         const char *projroot);
> +extern char *xf86openConfigFile(const char *path, const char *cmdline,
> +                               const char *projroot);
> +extern char *xf86openConfigDirFiles(const char *path, const char *cmdline,
> +                                   const char *projroot);
>  extern void xf86setBuiltinConfig(const char *config[]);
>  extern XF86ConfigPtr xf86readConfigFile(void);
>  extern void xf86closeConfigFile(void);

Seems pretty reasonable, but do you know why xf86openConfigDirFiles is
called twice? Oh, I guess it's because we also look at the system
config dir. Now I recall thinking this would leak but not really
caring to fix it at the time. :) I certainly like the idea of passing
around variables when possible and not poking globals.

These functions also get called from hw/xwin/winconfig.c. I'm not sure
how used that code is anymore, but I was trying to keep it updated at
the time. Besides that,

Reviewed-by: Dan Nicholson <dbn.lists at gmail.com>