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

[zhentan feng]

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?

[...]
>
> zhentan
-- 
Best wishes~