[PATCH v4l-utils] libdvbv5: leaks and double free in dvb_fe_open

Linux Media Controller development
 help / color / mirror / Atom feed

* [PATCH v4l-utils] libdvbv5: leaks and double free in dvb_fe_open_fname()
@ 2019-03-17 16:32 Sean Young
  2019-04-26 15:13 ` Mauro Carvalho Chehab
  0 siblings, 1 reply; 6+ messages in thread
From: Sean Young @ 2019-03-17 16:32 UTC (permalink / raw)
  To: linux-media; +Cc: Gregor Jasny

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.

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


^ permalink raw reply related	[flat|nested] 6+ messages in thread

* Re: [PATCH v4l-utils] libdvbv5: leaks and double free in dvb_fe_open_fname()
  2019-03-17 16:32 [PATCH v4l-utils] libdvbv5: leaks and double free in dvb_fe_open_fname() Sean Young
@ 2019-04-26 15:13 ` Mauro Carvalho Chehab
  2019-04-26 15:42   ` Mauro Carvalho Chehab
  0 siblings, 1 reply; 6+ messages in thread
From: Mauro Carvalho Chehab @ 2019-04-26 15:13 UTC (permalink / raw)
  To: Sean Young; +Cc: linux-media, Gregor Jasny

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

^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: [PATCH v4l-utils] libdvbv5: leaks and double free in dvb_fe_open_fname()
  2019-04-26 15:13 ` Mauro Carvalho Chehab
@ 2019-04-26 15:42   ` Mauro Carvalho Chehab
       [not found]     ` <CAJxGH0_bmRiGKCtOf_jFZh_wVsyKR4s1DoDcSvYF7UYx8JNS0g@mail.gmail.com>
  0 siblings, 1 reply; 6+ messages in thread
From: Mauro Carvalho Chehab @ 2019-04-26 15:42 UTC (permalink / raw)
  To: Sean Young, Gregor Jasny; +Cc: linux-media

Gregor,

This patch messed with all branches since stable-1.12. I applied the revert
patch already on all affected stable branches.

We should probably release a new fix for them soon.

Sorry for not looking this earlier.. I got some vacations earlier 
this month.

Regards,
Mauro

Em Fri, 26 Apr 2019 12:13:44 -0300
Mauro Carvalho Chehab <mchehab+samsung@kernel.org> escreveu:

> 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



Thanks,
Mauro

^ permalink raw reply	[flat|nested] 6+ messages in thread

[parent not found: <CAJxGH0_bmRiGKCtOf_jFZh_wVsyKR4s1DoDcSvYF7UYx8JNS0g@mail.gmail.com>]

* Re: [PATCH v4l-utils] libdvbv5: leaks and double free in dvb_fe_open_fname()
       [not found]     ` <CAJxGH0_bmRiGKCtOf_jFZh_wVsyKR4s1DoDcSvYF7UYx8JNS0g@mail.gmail.com>
@ 2019-04-26 18:49       ` Mauro Carvalho Chehab
  2019-05-03 13:00         ` Mauro Carvalho Chehab
  0 siblings, 1 reply; 6+ messages in thread
From: Mauro Carvalho Chehab @ 2019-04-26 18:49 UTC (permalink / raw)
  To: Gregor Jasny; +Cc: Sean Young, Linux Media Mailing List

Em Fri, 26 Apr 2019 18:08:22 +0200
Gregor Jasny <gjasny@googlemail.com> escreveu:

> Hello,
> 
> On Fri, 26 Apr 2019, 17:42 Mauro Carvalho Chehab, <
> mchehab+samsung@kernel.org> wrote:  
> 
> > Gregor,
> >
> > This patch messed with all branches since stable-1.12. I applied the revert
> > patch already on all affected stable branches.
> >
> > We should probably release a new fix for them soon.
> >
> > Sorry for not looking this earlier.. I got some vacations earlier
> > this month.
> >  
> 
> I also got a report in Debian:
> https://bugs.debian.org/cgi-bin/bugreport.cgi?bug=927341
> 
> I'm wondering if we could improve code quality by doing some automated
> testing. 

That's my dream as well.

> Is there a dummy dvb driver like vivi available?

Unfortunately, we don't have a full dummy dvb driver. There are some
dummy DVB frontend drivers, but they're really dummy do-nothing
drivers. We would need to add more stuff there for them to work, and
then add a virtual DVB driver on the top of it.

Doable, but requires someone with time for coding that.

To be fair, a DVB dummy driver is a lot easier than a V4L2 one, as
the DVB core already handles most of the stuff. The most complex
part would likely to use the vivid image generator converting it
into some video stream supported by DVB apps (the best would be
to encode it as MPEG-2 - as all apps support). We could use, on
a first version, some previously encoded video - or - even easier
- to just stream empty frames.

Thanks,
Mauro

^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: [PATCH v4l-utils] libdvbv5: leaks and double free in dvb_fe_open_fname()
  2019-04-26 18:49       ` Mauro Carvalho Chehab
@ 2019-05-03 13:00         ` Mauro Carvalho Chehab
  2019-05-03 13:02           ` Gregor Jasny
  0 siblings, 1 reply; 6+ messages in thread
From: Mauro Carvalho Chehab @ 2019-05-03 13:00 UTC (permalink / raw)
  To: Gregor Jasny; +Cc: Sean Young, Linux Media Mailing List

Hi Gregor,

Em Fri, 26 Apr 2019 15:49:17 -0300
Mauro Carvalho Chehab <mchehab+samsung@kernel.org> escreveu:

> Em Fri, 26 Apr 2019 18:08:22 +0200
> Gregor Jasny <gjasny@googlemail.com> escreveu:
> 
> > Hello,
> > 
> > On Fri, 26 Apr 2019, 17:42 Mauro Carvalho Chehab, <  
> > mchehab+samsung@kernel.org> wrote:    
> >   
> > > Gregor,
> > >
> > > This patch messed with all branches since stable-1.12. I applied the revert
> > > patch already on all affected stable branches.
> > >
> > > We should probably release a new fix for them soon.
> > >
> > > Sorry for not looking this earlier.. I got some vacations earlier
> > > this month.
> > >    
> > 
> > I also got a report in Debian:
> > https://bugs.debian.org/cgi-bin/bugreport.cgi?bug=927341


Are you planning to release a version 1.16.6 any time soon?

I could likely do it as well, but I would prefer if you can do,
as you know better the drills. We're still receiving new
complaints about this issue at Kaffeine's BZ. Just received
another one today.

Thanks,
Mauro

^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: [PATCH v4l-utils] libdvbv5: leaks and double free in dvb_fe_open_fname()
  2019-05-03 13:00         ` Mauro Carvalho Chehab
@ 2019-05-03 13:02           ` Gregor Jasny
  0 siblings, 0 replies; 6+ messages in thread
From: Gregor Jasny @ 2019-05-03 13:02 UTC (permalink / raw)
  To: Mauro Carvalho Chehab; +Cc: Sean Young, Linux Media Mailing List

Hello,

On 03.05.19 15:00, Mauro Carvalho Chehab wrote:
> Are you planning to release a version 1.16.6 any time soon?
> 
> I could likely do it as well, but I would prefer if you can do,
> as you know better the drills. We're still receiving new
> complaints about this issue at Kaffeine's BZ. Just received
> another one today.

Yes, I'll do it over the weekend. Spend the last week hiking in Austria 
and had to catch up at work.

Thanks,
Gregor

^ permalink raw reply	[flat|nested] 6+ messages in thread

end of thread, other threads:[~2019-05-03 13:02 UTC | newest]

Thread overview: 6+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2019-03-17 16:32 [PATCH v4l-utils] libdvbv5: leaks and double free in dvb_fe_open_fname() Sean Young
2019-04-26 15:13 ` Mauro Carvalho Chehab
2019-04-26 15:42   ` Mauro Carvalho Chehab
     [not found]     ` <CAJxGH0_bmRiGKCtOf_jFZh_wVsyKR4s1DoDcSvYF7UYx8JNS0g@mail.gmail.com>
2019-04-26 18:49       ` Mauro Carvalho Chehab
2019-05-03 13:00         ` Mauro Carvalho Chehab
2019-05-03 13:02           ` Gregor Jasny

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox