| Message ID | 20190317163220.1881-1-sean@mess.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [v4l-utils] libdvbv5: leaks and double free in dvb_fe_open_fname() | expand |
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
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
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
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
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
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;
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(-)