Re: [PATCH v4l-utils] libdvbv5: leaks and double free in dvb_fe_open_fname()

[Mauro Carvalho Chehab <mchehab+samsung@kernel.org>]

Em Sun, 17 Mar 2019 16:32:20 +0000
Sean Young <sean@mess.org> escreveu:

> dvb_fe_open_fname() takes ownership of fname if the function succeeds, but
> also in two of the error paths (e.g. if the ioctl FE_GET_PROPERTY fails).
> 
> Adjust dvb_fe_open_fname() so it copies fname rather than taking ownership
> (and passing that to params). This makes the code cleaner.

Just reverted this patch from stable-1.16, as it breaks Kaffeine.

There are two reports about the issue:

	https://bugs.kde.org/show_bug.cgi?id=406145
        https://bugzilla.redhat.com/show_bug.cgi?id=1695023

I was able to reproduce it locally.

So, better to keep a possible memory leak than to cause apps
to not function anymore.

> 
> Signed-off-by: Sean Young <sean@mess.org>
> ---
>  lib/libdvbv5/dvb-dev-local.c |  2 +-
>  lib/libdvbv5/dvb-fe.c        | 18 ++++++++----------
>  2 files changed, 9 insertions(+), 11 deletions(-)
> 
> diff --git a/lib/libdvbv5/dvb-dev-local.c b/lib/libdvbv5/dvb-dev-local.c
> index e98b967a..2de9a614 100644
> --- a/lib/libdvbv5/dvb-dev-local.c
> +++ b/lib/libdvbv5/dvb-dev-local.c
> @@ -467,7 +467,7 @@ static struct dvb_open_descriptor
>  			flags &= ~O_NONBLOCK;
>  		}
>  
> -		ret = dvb_fe_open_fname(parms, strdup(dev->path), flags);
> +		ret = dvb_fe_open_fname(parms, dev->path, flags);
>  		if (ret) {
>  			free(open_dev);
>  			return NULL;
> diff --git a/lib/libdvbv5/dvb-fe.c b/lib/libdvbv5/dvb-fe.c
> index 5dcf492e..7f634766 100644
> --- a/lib/libdvbv5/dvb-fe.c
> +++ b/lib/libdvbv5/dvb-fe.c
> @@ -133,7 +133,6 @@ struct dvb_v5_fe_parms *dvb_fe_open_flags(int adapter, int frontend,
>  					  int flags)
>  {
>  	int ret;
> -	char *fname;
>  	struct dvb_device *dvb;
>  	struct dvb_dev_list *dvb_dev;
>  	struct dvb_v5_fe_parms_priv *parms = NULL;
> @@ -153,7 +152,6 @@ struct dvb_v5_fe_parms *dvb_fe_open_flags(int adapter, int frontend,
>  		dvb_dev_free(dvb);
>  		return NULL;
>  	}
> -	fname = strdup(dvb_dev->path);
>  
>  	if (!strcmp(dvb_dev->bus_addr, "platform:dvbloopback")) {
>  		logfunc(LOG_WARNING, _("Detected dvbloopback"));
> @@ -161,14 +159,10 @@ struct dvb_v5_fe_parms *dvb_fe_open_flags(int adapter, int frontend,
>  	}
>  
>  	dvb_dev_free(dvb);
> -	if (!fname) {
> -		logfunc(LOG_ERR, _("fname calloc: %s"), strerror(errno));
> -		return NULL;
> -	}
> +
>  	parms = calloc(sizeof(*parms), 1);
>  	if (!parms) {
>  		logfunc(LOG_ERR, _("parms calloc: %s"), strerror(errno));
> -		free(fname);
>  		return NULL;
>  	}
>  	parms->p.verbose = verbose;
> @@ -183,7 +177,7 @@ struct dvb_v5_fe_parms *dvb_fe_open_flags(int adapter, int frontend,
>  	if (use_legacy_call)
>  		parms->p.legacy_fe = 1;
>  
> -	ret = dvb_fe_open_fname(parms, fname, flags);
> +	ret = dvb_fe_open_fname(parms, dvb_dev->path, flags);
>  	if (ret < 0) {
>  		dvb_v5_free(parms);
>  		return NULL;
> @@ -203,7 +197,6 @@ int dvb_fe_open_fname(struct dvb_v5_fe_parms_priv *parms, char *fname,
>  	fd = open(fname, flags, 0);
>  	if (fd == -1) {
>  		dvb_logerr(_("%s while opening %s"), strerror(errno), fname);
> -		free(fname);
>  		return -errno;
>  	}
>  
> @@ -224,7 +217,12 @@ int dvb_fe_open_fname(struct dvb_v5_fe_parms_priv *parms, char *fname,
>  		}
>  	}
>  
> -	parms->fname = fname;
> +	parms->fname = strdup(fname);
> +	if (!parms->fname) {
> +		dvb_logerr(_("fname calloc: %s"), strerror(errno));
> +		return -errno;
> +	}
> +
>  	parms->fd = fd;
>  	parms->fe_flags = flags;
>  	parms->dvb_prop[0].cmd = DTV_API_VERSION;



Thanks,
Mauro