[FFmpeg-devel] [PATCH] tcp.c/udp.c memleak?

[Ronald S. Bultje]

Hi Michael,

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
> places

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
utils.c:url_split().

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...

Ronald