[FFmpeg-devel] [PATCH] tcp.c/udp.c memleak?
Ronald S. Bultje
Sun Aug 24 00:28:56 CEST 2008
On Sat, Aug 23, 2008 at 5:19 PM, Michael Niedermayer <michaelni at gmx.at> wrote:
> On Sat, Aug 23, 2008 at 04:26:41PM -0400, Ronald S. Bultje wrote:
>> On Sat, Aug 23, 2008 at 4:08 PM, Michael Niedermayer <michaelni at gmx.at> wrote:
>> > On Sat, Aug 23, 2008 at 03:05:49PM -0400, Ronald S. Bultje wrote:
>> >> On Sat, Aug 23, 2008 at 1:43 PM, Ronald S. Bultje <rsbultje at gmail.com> wrote:
>> >> > time for more stuff. This patch removes the check for "@" in hostname
>> >> > for tcp.c, because url_split() already does that.
>> >> you forgot this one. :-). I've tested that even if auth is NULL, the @
>> >> part is stripped correctly, so this code is never reached. Do I need
>> >> to do additional testing?
>> > no, but if you replace the line by an equivalent assert() iam ok with
>> > it
>> That'd be exploitable if you give a URI with multiple @s?
> I suspect there are easier ways to make ffmpeg abort or exit.
> out of memory condition should be very easy to create at many
I'm not affraid of OOM, but if I give an URI with two @s in the
hostname, e.g. tcp://me at me at me:1, then url_split will put "me" in auth
and "me at me" in hostname. Only in those cases does the code in tcp.c do
something, but I don't think it's supposed to do what it does (just
think of what it'll do if your URI is tcp://me at me at me at me at me:1, it's
completely random). If you make it an assert(), then it'd abort on
URIs such as tcp://me at me at me:1, which is bad.
>> Change url_split() and add a return value?
> Iam not sure what you suggest and iam even less sure what the whole
> change really is doing.
So what I could do is modify url_split() such that it returns an error
if fed an URI with two @s in the hostname+auth part, such as
tcp://me at me at me:1. After that, the code above can be mofidied into an
assert(), but I don't think that's necessary anyway. See below.
> Its clear the line of code doesnt do nothing as the assert could trigger.
> So why was this line addded, was it never needed?
> has its need disapeared at some point?
> Was it always nonsense?
> If it was always nonsense then all code of the patch author should be
> carefully reviewed again ...
> What looks strange is that this line was added in the same revission that
> added url_split() there and that added the @ removing code in url_split()
I just noticed, that patch is nonsensical. I suppose the functionality
of that line in tcp.c was moved to url_split() but never removed from
tcp.c; then he submitted the patch, we didn't see and there we go.
You'll also notice the useless comments in that version of
Not sure how to figure out what other patches he submitted. "grep -r
PETR ." is empty here. We should probably just remove the
corresponding line from tcp.c...
More information about the ffmpeg-devel