[FFmpeg-devel] theora depayloader

[Martin Storsjö]

On Sat, 20 Mar 2010, Josh Allmann wrote:

> On 19 March 2010 21:19, Martin Storsj? <martin at martin.st> wrote:
> > On Fri, 19 Mar 2010, Josh Allmann wrote:
> >
> >
> >> + ? ? ? ? ? ?data_len += pkt_len;
> >> + ? ? ? ? ? ?len -= pkt_len+2;
> >> + ? ? ? }
> >> +
> >> + ? ? ? if (len < 0){
> >> + ? ? ? ? ?av_log(ctx, AV_LOG_ERROR,
> >> + ? ? ? ? ? ? ? ? "Length overread by %d\n", -len);
> >> + ? ? ? }
> >
> > Here, you could potentially get the case where len reaches exactly zero,
> > before i has reached num_pkts, so we'd get no warning even if the packet
> > was invalid...
> 
> Hm, good catch. Fixed by checking for len >= 0 in the for loop.
> 
> Now that I think of it, a bad packet could potentially give us a large
> negative pkt_len, which would in turn make len positive, but still
> invalid... so I changed the type declaration of pkt_len (and related
> header vars) to uint32_t.
> 
> I have not explicitly tested for these conditions, though.
> 
> Also added a return AVERROR_INVALIDDATA; I forgot that.


> +        // fast first pass to calculate total length
> +        for (i = 0, data_len = 0;  (i < num_pkts) && (len >= 0);  i++) {
> +            int off   = data_len + (i << 1);
> +            pkt_len   = AV_RB16(buf + off);
> +            data_len += pkt_len;
> +            len      -= pkt_len + 2;
> +        }
> +
> +        if (len < 0) {
> +            av_log(ctx, AV_LOG_ERROR, "Length overread by %d\n", -len);
> +            return AVERROR_INVALIDDATA;
> +        }

If len == 0 or 1, you will read outside of the buffer (which isn't a 
padded buffer IIRC). What about this loop condition instead:

for (...; i < num_pkts && len >= 2; i++)

and check for errors with:

if (len < 0 || i < num_pkts)

That is, if we've errored out due to the length restriction, i will be 
left at less than num_pkts, and we haven't parsed everything => error.


> +        pkt->stream_index = st->index;
> +
> +       // concatenate frames
> +        for (i = 0, write_len = 0; write_len < data_len; i++) {

Indentation off on the comment.

> +        if((res = url_open_dyn_buf(&data->fragment)) < 0)

Space after if

> +    num_packed         = bytestream_get_be32(&packed_headers);
> +    theora_data->ident = bytestream_get_be24(&packed_headers);
> +    length             = bytestream_get_be16(&packed_headers);

No checking if there's sufficient bytes to read - check e.g. 
packed_headers_end - packed_headers >= 9 before this.

> +int ff_theora_parse_fmtp_config(AVCodecContext * codec,
> +                                void *theora_data, char *attr, char *value)
> +{
> +  int result = 0;
> +  assert(codec->id == CODEC_ID_THEORA);
> +  assert(theora_data);
> +
> +  if (!strcmp(attr, "sampling")) {
> +    return AVERROR_PATCHWELCOME;
> +  } else if (!strcmp(attr, "width")) {

The indentation seems to be off in the whole function

// Martin