[FFmpeg-devel] [patch]MMS protocol over TCP

[Michael Niedermayer]

On Wed, Mar 31, 2010 at 11:25:45AM +0800, zhentan feng wrote:
> Hi
> 
> On Wed, Mar 31, 2010 at 5:23 AM, Michael Niedermayer <michaelni at gmx.at>wrote:
> 
> > On Wed, Mar 31, 2010 at 01:09:23AM +0800, zhentan feng wrote:
> > > Hi
> > >
> > > On Tue, Mar 30, 2010 at 6:42 AM, Michael Niedermayer <michaelni at gmx.at
> > >wrote:
> > >
> > > > On Sun, Mar 28, 2010 at 12:13:24AM +0800, zhentan feng wrote:
> > > > [...]
> > > > > +
> > > > > +/** Read incoming MMST media, header or command packet. */
> > > > > +static MMSSCPacketType get_tcp_server_response(MMSContext *mms)
> > > > > +{
> > > > > +    int read_result;
> > > > > +    MMSSCPacketType packet_type= -1;
> > > > > +    int done;
> > > > > +
> > > > > +    do {
> > > > > +        done= 1;
> > > > > +        if((read_result= url_read_complete(mms->mms_hd,
> > > > mms->incoming_buffer, 8))==8) {
> > > > > +            // handle command packet.
> > > > > +            if(AV_RL32(mms->incoming_buffer + 4)==0xb00bface) {
> > > > > +                mms->incoming_flags= mms->incoming_buffer[3];
> > > > > +                read_result= url_read_complete(mms->mms_hd,
> > > > mms->incoming_buffer+8, 4);
> > > > > +                if(read_result == 4) {
> > > > > +                    int length_remaining=
> > > > AV_RL32(mms->incoming_buffer+8) + 4;
> > > > > +
> > > > > +                    dprintf(NULL, "Length remaining is %d\n",
> > > > length_remaining);
> > > > > +                    // read the rest of the packet.
> > > > > +                    read_result = url_read_complete(mms->mms_hd,
> > > > mms->incoming_buffer + 12,
> > > > > +                                                  length_remaining)
> > ;
> > > > > +                    if (read_result == length_remaining) {
> > > > > +                        mms->incoming_buffer_length=
> > > > length_remaining+12;
> > > > > +                        packet_type=
> > AV_RL16(mms->incoming_buffer+36);
> > > > > +
> > > > > +                    } else {
> > > > > +                        dprintf(NULL, "3 read returned %d!\n",
> > > > read_result);
> > > > > +                    }
> > > > > +                } else {
> > > > > +                    dprintf(NULL, "2 read returned %d!\n",
> > read_result);
> > > > > +                }
> > > >
> > > > > +            } else {
> > > > > +                int length_remaining;
> > > > > +                int packet_id_type;
> > > > > +                int tmp;
> > > > > +
> > > > > +                assert(mms->pkt_buf_len==0);
> > > > > +
> > > > > +                //** VERIFY LENGTH REMAINING HAS SPACE
> > > > > +                // note we cache the first 8 bytes,
> > > > > +                // then fill up the buffer with the others
> > > > > +                tmp                       =
> > AV_RL16(mms->incoming_buffer
> > > > + 6);
> > > > > +                length_remaining          = (tmp - 8) & 0xffff;
> > > > > +                mms->incoming_packet_seq  =
> > > > AV_RL32(mms->incoming_buffer);
> > > > > +                packet_id_type            = mms->incoming_buffer[4];
> > > > > +                mms->incoming_flags       = mms->incoming_buffer[5];
> > > > > +                mms->pkt_buf_len          = length_remaining;
> > > > > +                mms->pkt_read_ptr         = mms->incoming_buffer;
> > > > > +
> > > > > +                read_result= url_read_complete(mms->mms_hd,
> > > > mms->incoming_buffer, length_remaining);
> > > >
> > > > is there any check for not overwriting the array bounds?
> > > >
> > > >
> > > yes, you are right. Since it maybe a error happened when overwriting.
> > > I will add some debug info and return error code directly.
> > >
> > >
> > > > > +                if(read_result != length_remaining) {
> > > > > +                    dprintf(NULL, "read_bytes result: %d asking for
> > > > %d\n",
> > > > > +                            read_result, length_remaining);
> > > > > +                    break;
> > > > > +                } else {
> > > > > +                    // if we successfully read everything.
> > > > > +                    if(packet_id_type == mms->header_packet_id) {
> > > > > +                        packet_type = SC_PKT_ASF_HEADER;
> > > > > +                        // Store the asf header
> > > > > +                        if(!mms->header_parsed) {
> > > >
> > > > > +                            mms->asf_header =
> > > > av_realloc(mms->asf_header,
> > > > > +                                              mms->asf_header_size
> > > > > +                                              + mms->pkt_buf_len);
> > > >
> > > > missing check for realloc failure
> > > > also can mms->asf_header_size + mms->pkt_buf_len overflow? if so it
> > > > must be checked
> > > >
> > > >
> > > overwriting wouldn't happen here, because we actually realloc the size we
> > > will copy to.
> >
> > the addition of the integers mms->asf_header_size and mms->pkt_buf_len
> > could maybe overflow?
> > and if this is possible too little would be allocated
> >
> >
>  first, I'll check the failure of realloc.
>  if realloc succeed, the asf_header buffer size change
> from mms->asf_header_size to mms->asf_header_size +  mms->pkt_buf_len.
>  (at the first time mms->asf_header_size is zero obviously).
>  then we will copy extra mms->pkt_buf_len data to mms->asf_header.
>  IMHO, there is no overflow.
>  isn't it? or maybe I misunderstood what you point out?

if you add 2 unsigned numbers 0xFFFFFFFF + 1
you get 0 on a 32bit system, which is smaller than both
I do not know if this can happen here, but if it can happen then
the allocated buffer will be smaller than it should be
and writing 0xFFFFFFFF bytes into it and then 1 byte will fail

[...]

-- 
Michael     GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB

When you are offended at any man's fault, turn to yourself and study your
own failings. Then you will forget your anger. -- Epictetus
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 189 bytes
Desc: Digital signature
URL: <http://lists.mplayerhq.hu/pipermail/ffmpeg-devel/attachments/20100331/aaf75382/attachment.pgp>