[FFmpeg-devel] [PATCH] Add a RTP depacketizer for the X-Qt format

[Ronald S. Bultje]

Hi,

On Tue, Oct 5, 2010 at 6:07 PM, Martin Storsj? <martin at martin.st> wrote:
> On Tue, 5 Oct 2010, Ronald S. Bultje wrote:
>> On Tue, Oct 5, 2010 at 5:47 PM, Martin Storsj? <martin at martin.st> wrote:
>> > On Tue, 5 Oct 2010, Ronald S. Bultje wrote:
>> >> On Sun, Oct 3, 2010 at 11:45 AM, Martin Storsj? <martin at martin.st> wrote:
>> >> > This is the first brushed up version of the depacketizer for the X-Qt
>> >> > format (i.e., any quicktime supported format, in RTP), based on earlier
>> >> > work by Ronald.
>> >> [..]
>> >> > + ? ? ? ?if (qt->pkt.size > 0 && qt->timestamp == *timestamp) {
>> >> > +void* ptr = qt->pkt.data;
>> >> > + ? ? ? ? ? ?qt->pkt.data = av_realloc(qt->pkt.data, qt->pkt.size + alen + FF_INPUT_BUFFER_PADDING_SIZE);
>> >> > + ? ? ? ?} else {
>> >>
>> >> What is the void ptr doing there?
>> >
>> > It's stray debug code that I accidentally left there, I think I mentioned
>> > it on irc the other day that I had cleaned it up locally.
>> >
>> >> I think the code needs some length-checks, there's some possible
>> >> buffer overreads (and possible overwrites as a result of that in the
>> >> memcpy()s).
>> >
>> > I've done a lot of improvments regarding such things compared to your
>> > version that I started from - I'm quite confident with this version
>> > actually. It passes valgrind without any warnings or leaks.
>> [..]
>> > + ? ?switch (packing_scheme) {
>> > + ? ?case 3: { /* one data packet spread over 1 or multiple RTP packets */
>> > + ? ? ? ?int alen = len - url_ftell(&pb);
>>
>> At this point, alen can be negative which isn't checked in this part
>> of the code.
>>
>> [..]
>> > + ? ? ? ?memcpy(qt->pkt.data + qt->pkt.size, buf + url_ftell(&pb), alen);
>>
>> And would then crash here.
>
> Good catch. Checking if (alen <= 0) as the other switch case already does.
[..]
> +        data_len = get_bits(&gb, 16);
> +
> +        url_fseek(&pb, pos + 4, SEEK_SET);
> +        tag = get_le32(&pb);
> +        if ((st->codec->codec_type == CODEC_TYPE_VIDEO &&
> +                 tag != MKTAG('v','i','d','e')) ||
> +            (st->codec->codec_type == CODEC_TYPE_AUDIO &&
> +                 tag != MKTAG('s','o','u','n')))
> +            return AVERROR_INVALIDDATA;
> +        av_set_pts_info(st, 32, 1, get_be32(&pb));
> +
> +        /* TLVs */
> +        while (url_ftell(&pb) < pos + data_len) {

I wonder what happens if data_len is a random value (e.g. 0xFFFF) that
is much bigger than the actual buffer size. It should probably be
checked also to not exceed the input buffer size (len).

Ronald