[MPlayer-dev-eng] [PATCH][BUG] Incorrect memleak fix code in input/input.c might cause incorrect free

[Shachar Raindel]

On Tue, 3 Aug 2004 11:14:11 +0200, Alexander Strasser <eclipse7 at gmx.net> wrote:
> On Tue, Aug 03, 2004 at 11:54:38AM +0300, Shachar Raindel wrote:
> > On Tue, 3 Aug 2004 10:24:51 +0200, Alexander Strasser <eclipse7 at gmx.net> wrote:
> > > OK, i knew i had some doubts and now i found why.
> > > Please look at the attached patch and comment.
> >
> > Yep, I agree that this was a bug in my original patch. What about this
> > one? It is a bit more elegant IMHO, and you have only a single point
> > in which you free this chunk of mem, which makes it a tiny little bit
> > more clear... Don't know if this breaks buggy GCC compilers, and if I
> > should care.
> Dunno if it is really more elegant or more easy to follow.
> Maybe someone else can say something about it ( maybe you
> are right and one freeing point is better, thought about
> that too ).

Both of the patches are correct. Choose one and apply it. Mine has the
benefit of slightly smaller code, but it might break on some buggy
compilers. You is a bit more difficult to understand, but there is
more chance it will compile on buggy compilers. Choose as you wish,
and apply something. This is a probably exploitable bug, which might
allow users on systems in which mplayer is installed as suid to get
root. I don't think we need to publish a bulletin about this, as there
is a very large recommendation against installing mplayer as suid on
any multiuser system, but it is not good to have such bugs unpatched
for more than a week since it was first reported.

Soon we will start to release only monthly patch-pack, to "ease the
administrators job" like some other well know companies on the
market..... :-)


> > Also, any particullar reason for having a prototype for get_path
> > inside the file, instead of having a single prototype for all of the
> > files that uses get_path, in a header file?
> This is a mess IMHO. I noticed it while trying to fix these memleaks.
> It is inlcuded in more than one file as get_path.c and is declared
> extern or as a prototype or however you want to say it in some other
> places too.
> 
> I think this should really be cleaned up... i'll start another thread
> about it after this is fixed.
> 
This is a general problem in mplayer, probably due to the fact that it
evolved without design in the first place. The include files are a
total mess, and some of them must be included only after certain
variables has been defined IN THE C FILE INCLUDING THEM. as a result,
developer prefer to put an "extern"/prototype line in their file,
possibly allowing   bugs caused by changes in the called function
parameters list/changes in the variable type to go unnoticed, and make
these developers later tear their few remaining hairs and bash their
head against the wall, until they find out that someone added a
parameter and they missed the cvslog message.... Somebody needs to
clean-up this mess.... Soon....

    Cheers,
    Shachar