[MPlayer-dev-eng] [PATCH] Fix memleak in stream_dvb.c (and libmenu patch)

[Andrew Calkin]

2007/12/15, Nico Sabbi <Nicola.Sabbi at poste.it>:
> Il Friday 14 December 2007 18:50:56 Andrew Calkin ha scritto:
> > Hi,
> >
> > The following patch fixes a memleak in stream/stream_dvb.c, where
> > the dvb_config memory was never freed. It needed a slight rework
> > of libmenu's dvb menu, because it also called on get_dvb_config(),
> > so I modified that too. The patches are attached, plz review and
> > comment.
> >
> > //Andrew
> > dvb_stream.diff
> >   --- MPlayer/stream/stream_dvb.c2007-12-14 14:23:05.000000000
> > +0900 +++ MPlayer/stream/stream_dvb.c2007-12-14 15:30:13.000000000
> > +0900 @@ -605,6 +605,8 @@
> >
> > +
> > +dvb_config_t *dvb_dup_config(dvb_config_t *config)
> > +{
> > + int i;
> > + dvb_config_t *new_config;
> > +
> > + if(!config || config->count == 0)
> > + return NULL;
> > +
> > + new_config = calloc(1, sizeof(dvb_config_t));
> > + if (!new_config)
> > + return NULL;
> > +
> > + new_config->count = config->count;
> > + new_config->cards = calloc(config->count,
> > sizeof(dvb_card_config_t)); +
> > + for (i=0; i<new_config->count; i++)
> > + {
> > + new_config->cards[i] = config->cards[i];
> > + if (config->cards[i].name)
> > + new_config->cards[i].name = strdup(config->cards[i].name);
> > + if (config->cards[i].list)
> > + {
> > + new_config->cards[i].list =
> > malloc(sizeof(dvb_channels_list)); + if
> > (!new_config->cards[i].list)
> > + continue;
> > + memcpy(new_config->cards[i].list, config->cards[i].list,
> > + sizeof(dvb_channels_list));
> > + if (config->cards[i].list->channels)
> > + {
> > + new_config->cards[i].list->channels =
> > malloc(sizeof(dvb_channel_t)); + if
> > (!new_config->cards[i].list->channels)
> > + continue;
> > + memcpy(new_config->cards[i].list->channels,
> > + config->cards[i].list->channels,
> > + sizeof(dvb_channel_t));
> > + }
>
> this is wrong for 2 reasons:
> -you are duplicating in new_config some memory malloc()ated in
> dvb_get_config() including the old pointers

As I see it, I then overwrite this old ptr with malloc calls, thus
setting the pointer to the newly acquired memory location. Then I
memcpy() from the old location to the newly allocated memory. Am I
wrong? Plz see lines 77 and 80, and lines 84 and 87.

> - the .name should be strdup()ed

Isn't that what I submitted already? Plz see line 74 of the patch file.

> Also, better call dvb_get_config() a second time in menu_dvbin.c and
> removing the file-static variable in stream_dvb.c rather than adding
> so much game with memory in stream_dvb.c

Ok, that can be done easily enough, if you prefer. I thought the
motivation for the previous approach was to avoid having to parse the
files again if already done once, but if you don't care then that is
just as simple- removal of the dvb_dup_config() and the line referring
to the call of the function in dvb_get_config(). If that is indeed
what you want, plz state so and I'll resend the patch. I just want
clarification of what I did wrong in terms of ptr management, as you
stated earlier (in case I overlooked something).

//Andrew