[syslinux] [PATCH v1 1/1] extlinux: fix memory leak

[Paulo Alcantara]

On Wed,  9 Sep 2015 14:00:35 +0300
Imran Zaman via Syslinux <syslinux at zytor.com> wrote:

> devname is put on heap for all cases to avoid memory
> leak, and ease of use in future as well
> 
> Signed-off-by: Imran Zaman <imran.zaman at intel.com>
> ---
>  extlinux/main.c | 48 ++++++++++++++++++++++++++++++++----------------
>  1 file changed, 32 insertions(+), 16 deletions(-)
> 
> diff --git a/extlinux/main.c b/extlinux/main.c
> index 55a1495..7bb7443 100644
> --- a/extlinux/main.c
> +++ b/extlinux/main.c
> @@ -1044,12 +1044,12 @@ err:
>  }
>  
>  #ifndef __KLIBC__
> -static const char *find_device(const char *mtab_file, dev_t dev)
> +static char *find_device(const char *mtab_file, dev_t dev)

Please, keep it constant. There is no real point to remove its
constantness, right? If I'm not missing anything here, please do the
same for the others.

>  {
>      struct mntent *mnt;
>      struct stat dst;
>      FILE *mtab;
> -    const char *devname = NULL;
> +    char *devname = NULL;
>      bool done;
>  
>      mtab = setmntent(mtab_file, "r");
> @@ -1131,7 +1131,7 @@ static const char *find_device(const char
> *mtab_file, dev_t dev)
>   * On newer Linux kernels we can use sysfs to get a backwards mapping
>   * from device names to standard filenames
>   */
> -static const char *find_device_sysfs(dev_t dev)
> +static char *find_device_sysfs(dev_t dev)
>  {
>      char sysname[64];
>      char linkname[PATH_MAX];
> @@ -1281,9 +1281,20 @@ err:
>      return rv;
>  }
>  
> -static const char *get_devname(const char *path)
> +static char *dupname(const char *name)
>  {
> -    const char *devname = NULL;
> +    char *out = NULL;
> +    int len = 0;
> +    if (name)
> +        len = strlen(name);
> +    if (len > 0)
> +        out = strndup(name, len);
> +    return out;
> +}
> +
> +static char *get_devname(const char *path)
> +{
> +    char *devname = NULL;
>      struct stat st;
>      struct statfs sfs;
>  
> @@ -1297,22 +1308,22 @@ static const char *get_devname(const char
> *path) }
>  
>      if (opt.device)
> -	devname = opt.device;
> +	devname = strdup(opt.device);
>  
>      if (!devname){
>  	if (fs_type == BTRFS) {
>  	    /* For btrfs try to get the device name from btrfs
> itself */
> -	    devname = find_device_btrfs(path);
> +	    devname = dupname(find_device_btrfs(path));
>  	}
>      }
>  
>      if (!devname) {
> -	devname = find_device_mountinfo(path, st.st_dev);
> +	devname = dupname(find_device_mountinfo(path, st.st_dev));
>      }
>  
>  #ifdef __KLIBC__
>      if (!devname) {
> -	devname = find_device_sysfs(st.st_dev);
> +	devname = dupname(find_device_sysfs(st.st_dev));

find_device_sysfs() already returns an allocated pointer to the device
name, so you're leaking memory here -- no need to call dupname(). 

>      }
>      if (!devname) {
>  	/* klibc doesn't have getmntent and friends; instead, just
> create @@ -1326,7 +1337,7 @@ static const char *get_devname(const
> char *path) }
>  
>  	atexit(device_cleanup);	/* unlink the device node on
> exit */
> -	devname = devname_buf;
> +	devname = dupname(devname_buf);
>      }
>  
>  #else
> @@ -1338,7 +1349,7 @@ static const char *get_devname(const char *path)
>          devname = find_device("/etc/mtab", st.st_dev);
>      }
>      if (!devname) {
> -	devname = find_device_sysfs(st.st_dev);
> +	devname = dupname(find_device_sysfs(st.st_dev));

Ditto.

>  
>  	fprintf(stderr, "%s: cannot find device for path %s\n",
> program, path); return devname;
> @@ -1350,10 +1361,10 @@ static const char *get_devname(const char
> *path) return devname;
>  }
>  
> -static int open_device(const char *path, struct stat *st, const char
> **_devname) +static int open_device(const char *path, struct stat
> *st, char **_devname) {
>      int devfd;
> -    const char *devname = NULL;
> +    char *devname = NULL;
>      struct statfs sfs;
>  
>      if (st)
> @@ -1393,11 +1404,10 @@ static int open_device(const char *path,
> struct stat *st, const char **_devname) 
>      devfd = -1;
>      devname = get_devname(path);
> -    if (_devname)
> -	*_devname = devname;
>  
>      if ((devfd = open(devname, O_RDWR | O_SYNC)) < 0) {
>  	fprintf(stderr, "%s: cannot open device %s\n", program,
> devname);
> +	free(devname);
>  	return -1;
>      }
>  
> @@ -1405,9 +1415,14 @@ static int open_device(const char *path,
> struct stat *st, const char **_devname) if (validate_device(path,
> devfd)) { fprintf(stderr, "%s: path %s doesn't match device %s\n",
>  		program, path, devname);
> +	free(devname);
>  	close(devfd);
>  	return -1;
>      }
> +    if (_devname)
> +    *_devname = devname;
> +    else
> +    free(devname);

Please, indent this snippet of code.

Thanks,

Paulo

>      return devfd;
>  }
>  
> @@ -1468,7 +1483,7 @@ static int install_loader(const char *path, int
> update_only) {
>      struct stat st, fst;
>      int devfd, rv;
> -    const char *devname;
> +    char *devname = NULL;
>  
>      devfd = open_device(path, &st, &devname);
>      if (devfd < 0)
> @@ -1508,6 +1523,7 @@ static int install_loader(const char *path, int
> update_only) 
>      sync();
>      rv = install_bootblock(devfd, devname);
> +    free(devname);
>      close(devfd);
>      sync();
>  


-- 
Paulo Alcantara, C.E.S.A.R
Speaking for myself only.