[FFmpeg-devel] [PATCH] ACT demuxer

[Vladimir Voroshilov]

Hi, Reimar

On Sun, Feb 24, 2008 at 4:56 PM, Reimar D?ffinger
<Reimar.Doeffinger at stud.uni-karlsruhe.de> wrote:

> [...]

>  > +static int act_probe(AVProbeData *p)
>  > +{
>  > +    if ((AV_RL32(&p->buf[0]) != RIFF_TAG) ||
>  > +        (AV_RL32(&p->buf[8]) != WAVE_TAG) ||
>  > +        (AV_RL32(&p->buf[16]) != 16))
>  > +    return 0;
>  > +
>  > +    if(p->buf[256]!=0x84)
>  > +        return 0;
>
>  IIRC the buffer is only guaranteed to be 32 bytes at least, so here you
>  must check buf_size.

Added buf_size and filename extension checks.

>  > +static int act_read_header(AVFormatContext *s,
>  > +                           AVFormatParameters *ap)
>
> > +{
>  > +    ACTContext* ctx = s->priv_data;
>  > +    ByteIOContext *pb = s->pb;
>  > +    int size;
>  > +    AVStream* st;
>
>  * is placed inconsistently.

Fixed.

>  > +    ctx->hdr.tag=get_byte(pb);
>  > +    if(ctx->hdr.tag!=0x84)
>  > +        return AVERROR_INVALIDDATA;
>
>  Does it have any advantage to check here as well?
>  If there ever comes up some new variant that for no good reason uses
>  0x85 instead of 0x84 for tag but leaves everything the same,
>  this check means you cannot even force this demuxer manually.

Removed

>  > +    st->codec->codec_tag = 0;
>
>  There is no need to set it to 0, it is initialized to that.

Fixed.

>  > +    st->duration=(ctx->hdr.minutes*60+ctx->hdr.sec)*100+ctx->hdr.msec/10;
>  > +
>  > +    av_set_pts_info(st, 64, 1, 800);
>
>  Duration is in time_base units, your time_base is 1/800s but you set
>  duration in 1/100s units, so your duration value should be off by a
>  factor 8?
>  Also, since it is only used here in this function, is it not fairly
>  pointless and a (tiny) waste of memory to have the header in the context?

time_base was set to duration of one packet along with pkt->duration=0
This makes pts calculation a bit simple even for two formats.

>  > +static int act_read_packet(AVFormatContext *s,
>  > +                          AVPacket *pkt)
>  > +{
>  > +    ACTContext* ctx = s->priv_data;
>  > +    ByteIOContext *pb = s->pb;
>  > +    uint8_t bFrame[22];
>  > +    uint8_t *pkt_buf;
>  > +    int bytes_read;
>
> > +    int frame_size=s->streams[0]->codec->frame_size;
>  > +
>  > +    bytes_read = get_buffer(pb, bFrame, frame_size);
>
>  There are all kinds of weird assumptions here.
>  Since bFrame is 22 byte large (why anyway?), you assume that frame_size is <= 22,
>  without checking.
>  Maybe there is no risk currently, but it certainly makes it excessively
>  easy to accidentally introduce an exploit later.
>  Also since the code below and in general assumes that frame_size is 10
>  anyway, why not just #define FRAME_SIZE 10 and use that?

Fixed by using av_get_packet  along with in-place bytes swapping.

>  > +    if(bytes_read != frame_size || av_new_packet(pkt, frame_size))
>  > +        return AVERROR(EIO);
>
>  AVERROR(EIO) is wrong when av_new_packet fails. Actually you should just
>  return the error av_new_packet gave.

Separated.

>  > +    pkt_buf=(uint8_t*)pkt->data;
>
>  The cast should not be necessary at all.
>  Actually, since pkt->data is uint8_t * already IMO just use it directly.
>
>
>  > +    pkt_buf[1]=bFrame[0];
>  > +    pkt_buf[3]=bFrame[1];
>  > +    pkt_buf[5]=bFrame[2];
>  > +    pkt_buf[7]=bFrame[3];
>  > +    pkt_buf[9]=bFrame[4];
>  > +    pkt_buf[0]=bFrame[5];
>  > +    pkt_buf[2]=bFrame[6];
>  > +    pkt_buf[4]=bFrame[7];
>  > +    pkt_buf[6]=bFrame[8];
>  > +    pkt_buf[8]=bFrame[9];
>
>  Hmm.. could by done in-place, has the advantage that
>  av_get_packet could be used, but it is rather obfuscated...
>
>  tmp = pkt->data[0];
>  pkt->data[0] = pkt->data[5];
>  pkt->data[5] = pkt->data[2];
>  pkt->data[2] = pkt->data[6];
>  pkt->data[6] = pkt->data[8];
>  pkt->data[8] = pkt->data[9];
>  pkt->data[9] = pkt->data[4];
>  pkt->data[4] = pkt->data[7];
>  pkt->data[7] = pkt->data[3];
>  pkt->data[3] = pkt->data[1];
>  pkt->data[1] = tmp;

See above.

Hopefully attached patch is better.


-- 
Regards,
Vladimir Voroshilov     mailto:voroshil at gmail.com
JID: voroshil at gmail.com, voroshil at jabber.ru
ICQ: 95587719
-------------- next part --------------
An embedded and charset-unspecified text was scrubbed...
Name: act_03.diff
URL: <http://lists.mplayerhq.hu/pipermail/ffmpeg-devel/attachments/20080224/65208a50/attachment.txt>