[MPlayer-dev-eng] [PATCH][Trac 2311] codec_path mem leak

[Alexander Strasser]

Hi!

On 2017-01-17 09:25 +0900, KO Myung-Hun wrote:
> Alexander Strasser wrote:
> > 
> > while investigating the leaks pointed out in ticket #2311, I found
> > that the attached patch fixes the remaining leak.
> > 
> > The current code seems pretty wrong. At least, as I understand it,
> > the function set_codec_path ever only got called once. The implementation
> > combined with exporting the codec_path global var seems not solid at all.
> > 
> > Still I feel like I am missing something...
> > It seems to easy to be true, maybe some of you can have a look or test.
> > I only tested by debug printing the value of codec_path after common_init
> > in mplayer.c, and not actually loading a binary codec.
> > 
> > 
> 
> How about calling set_codec_path(0) at exit ?

  Unfortunately that will not help I fear.MPlayer's config system will
overwrite the reference to the original memory before set_codec_path is
even called.

> > 0001-mpcommon-common_init-Avoid-creating-a-mem-leak-of-co.patch
> > 
> > 
> > From 574fb28e7f6411300776d06bc62d23c5bb82da24 Mon Sep 17 00:00:00 2001
> > From: Alexander Strasser <eclipse7 at gmx.net>
> > Date: Wed, 4 Jan 2017 23:45:30 +0100
> > Subject: [PATCH] mpcommon/common_init: Avoid creating a mem leak of codec_path
> > 
> > The codec path is set by the config system anyway. Setting it again
> > and assuming the original value wasn't already duplicated by the
> > config system just creates a mem leak.
> > 
> > This change also simplifies the code because function set_codec_path
> > isn't needed anymore.
> > 
> > Fixes part of ticket 2311
> > ---
> >  mpcommon.c |  3 ---
> >  path.c     | 16 ----------------
> >  path.h     |  1 -
> >  3 files changed, 20 deletions(-)
> > 
> > diff --git a/mpcommon.c b/mpcommon.c
> > index ebc9711e6..211828c49 100644
> > --- a/mpcommon.c
> > +++ b/mpcommon.c
> > @@ -588,9 +588,6 @@ int common_init(void)
> >      set_priority();
> >  #endif
> >  
> > -    if (codec_path)
> > -        set_codec_path(codec_path);
> > -

  Utilizing needs_free by initializing it to 1 will be even worse; because
as can be deduced from above lines of code, that would free the reference
string which is to be copied to the newly allocated variable as written
in the code chunk below.

  I can't see why the above call makes any sense. Why should we call 
a function with codec_path to, er, set codec_path to, hmm, codec_path ?
In which all codec_path are the same variable in the data section.

  As it stands it look like a refactoring error, but I did not yet search
the version history to confirm my guess.


> >      /* Check codecs.conf. */
> >      if (!codecs_file || !parse_codec_cfg(codecs_file)) {
> >          char *conf_path = get_path("codecs.conf");
> > diff --git a/path.c b/path.c
> > index b8d9335fe..e6287d220 100644
> > --- a/path.c
> > +++ b/path.c
> > @@ -174,22 +174,6 @@ void set_path_env(void)
> >  
> >  char *codec_path = BINARY_CODECS_PATH;
> >  
> > -static int needs_free = 0;
> > -
> > -void set_codec_path(const char *path)
> > -{
> > -    if (needs_free)
> > -        free(codec_path);
> > -    if (path == 0) {
> > -        codec_path = BINARY_CODECS_PATH;
> > -        needs_free = 0;
> > -        return;
> > -    }
> > -    codec_path = malloc(strlen(path) + 1);
> > -    strcpy(codec_path, path);
> > -    needs_free = 1;
> > -}
> > -
> >  /**
> >   * @brief Returns the basename substring of a path.
> >   */
> > diff --git a/path.h b/path.h
> > index 1389fc491..0f4594340 100644
> > --- a/path.h
> > +++ b/path.h
> > @@ -25,7 +25,6 @@ extern char *codec_path;
> >  
> >  char *get_path(const char *filename);
> >  void set_path_env(void);
> > -void set_codec_path(const char *path);
> >  const char *mp_basename(const char *path);
> >  char *mp_dirname(const char *path);
> >  char *mp_path_join(const char *base, const char *new_path);
> > 


  Alexander