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

zhentan feng spyfeng
Wed May 19 18:41:16 CEST 2010


hi

On Wed, May 19, 2010 at 7:48 AM, Michael Niedermayer <michaelni at gmx.at>wrote:

> On Wed, May 12, 2010 at 12:11:37AM +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;
> > +
> > +    for(;;) {
> > +        if((read_result= url_read_complete(mms->mms_hd, mms->in_buffer,
> 8))==8) {
> > +            // handle command packet.
> > +            if(AV_RL32(mms->in_buffer + 4)==0xb00bface) {
> > +                mms->incoming_flags= mms->in_buffer[3];
> > +                read_result= url_read_complete(mms->mms_hd,
> mms->in_buffer+8, 4);
> > +                if(read_result == 4) {
> > +                    int length_remaining= AV_RL32(mms->in_buffer+8) + 4;
> > +
> > +                    dprintf(NULL, "Length remaining is %d\n",
> length_remaining);
> > +                    // read the rest of the packet.
> > +                    if (length_remaining < 0
> > +                        || length_remaining > sizeof(mms->in_buffer) -
> 12) {
> > +                        dprintf("Incoming message len %d exceeds buffer
> len %d\n",
> > +                            length_remaining, sizeof(mms->in_buffer) -
> 12);
> > +                        break;
>
> all breaks that are supposed to result in a return -1 should be replaced
> by a litteral return -1 as this is less fragile
>
>
> > +                    }
> > +                    read_result = url_read_complete(mms->mms_hd,
> mms->in_buffer + 12,
> > +                                                  length_remaining) ;
> > +                    if (read_result == length_remaining) {
> > +                        packet_type= AV_RL16(mms->in_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->remaining_in_len==0);
> > +
> > +                // note we cache the first 8 bytes,
> > +                // then fill up the buffer with the others
> > +                tmp                       = AV_RL16(mms->in_buffer + 6);
> > +                length_remaining          = (tmp - 8) & 0xffff;
> > +                mms->incoming_packet_seq  = AV_RL32(mms->in_buffer);
> > +                packet_id_type            = mms->in_buffer[4];
> > +                mms->incoming_flags       = mms->in_buffer[5];
> > +
> > +                if (length_remaining < 0
> > +                        || length_remaining > sizeof(mms->in_buffer)) {
> > +                    dprintf("Incoming data len %d exceeds buffer len
> %d\n",
> > +                            length_remaining, sizeof(mms->in_buffer));
> > +                    break;
> > +                }
> > +                mms->remaining_in_len    = length_remaining;
> > +                mms->read_in_ptr         = mms->in_buffer;
> > +                read_result= url_read_complete(mms->mms_hd,
> mms->in_buffer, length_remaining);
> > +                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) {
> > +                            void *p = av_realloc(mms->asf_header,
> > +                                              mms->asf_header_size
> > +                                              + mms->remaining_in_len);
> > +                            if (!p) {
> > +                                av_freep(&mms->asf_header);
> > +                                return AVERROR(ENOMEM);
> > +                            }
> > +                            mms->asf_header = p;
> > +                            memcpy(mms->asf_header +
> mms->asf_header_size,
> > +                                                 mms->read_in_ptr,
> > +                                                 mms->remaining_in_len);
> > +                            mms->asf_header_size +=
> mms->remaining_in_len;
> > +                        }
> > +                    } else if(packet_id_type == mms->packet_id) {
> > +                        packet_type = SC_PKT_ASF_MEDIA;
> > +                    } else {
> > +                        dprintf(NULL, "packet id type %d is old.",
> packet_id_type);
> > +                        continue;
> > +                    }
> > +                }
> > +            }
>
> > +            break;
> > +        } else {
> > +            if(read_result<0) {
> > +                dprintf(NULL, "Read error (or cancelled) returned
> %d!\n", read_result);
> > +                packet_type = SC_PKT_CANCEL;
> > +            } else {
> > +                dprintf(NULL, "Read result of zero?!\n");
> > +                packet_type = SC_PKT_NO_DATA;
> > +            }
> > +            return packet_type;
>
> you mix break and return packet_type, and break just results in
> return packet_type
> this is slightly confusing and inconsistent
>
>
> > +        }
> > +    }
> > +    return packet_type;
>
>
>
> [...]
> > +recv_again:
> > +    type = get_tcp_server_response(mms);
> > +    if (type != expect_type) {
> > +        if (type == SC_PKT_KEEPALIVE) {
> > +            send_keepalive_packet(mms);
> > +            goto recv_again;
> > +        } else if (type == SC_PKT_STREAM_CHANGING) {
> > +            handle_packet_stream_changing_type(mms);
> > +            //TODO: Handle new header when change the stream type.
> > +            return -1;
> > +        } else {
> > +            dprintf(NULL,"Unhandled packet type %d\n", type);
> > +            return -1;
> > +        }
> > +    } else {
>
> > +        if (type == SC_PKT_ASF_MEDIA)
> > +            pad_media_packet(mms);
>
> is there a reason why this is not done in get_tcp_server_response() ?
>
>
> > +        return 0;
> > +    }
>
> this should be a loop of some kind and continue instead of goto
>
>
> [...]
> > +static int asf_header_parser(MMSContext *mms)
> > +{
> > +    uint8_t *p = mms->asf_header, *end = mms->asf_header +
> mms->asf_header_size;
> > +    int flags, stream_id;
> > +    mms->stream_num = 0;
> > +
> > +    if (mms->asf_header_size < sizeof(ff_asf_guid) * 2 + 22 ||
> > +        memcmp(p, ff_asf_header, sizeof(ff_asf_guid)))
> > +        return -1;
> > +
> > +    p += sizeof(ff_asf_guid) + 14;
> > +    do {
> > +        uint64_t chunksize = AV_RL64(p + sizeof(ff_asf_guid));
> > +        if (!memcmp(p, ff_asf_file_header, sizeof(ff_asf_guid))) {
> > +            /* read packet size */
> > +            if (end - p > sizeof(ff_asf_guid) * 2 + 68) {
> > +                mms->asf_packet_len = AV_RL32(p + sizeof(ff_asf_guid) *
> 2 + 64);
> > +                if (mms->asf_packet_len <= 0 || mms->asf_packet_len >
> sizeof(mms->in_buffer)) {
> > +                    dprintf(NULL,"Too large packet len:%d"
> > +                        " may overwrite in_buffer when padding",
> mms->asf_packet_len);
> > +                    return -1;
>
> you write an invalid value in asf_packet_len and you never check the
> return value of this function
> please take security more serious, overwriting the end of arrays
> is a serious issue you must check your code and make sure that
> no such or other security issues remain!
>
>
> yes, my fault.
I'll review the file carefully again.
thanks


> > +                }
> > +            }
> > +        } else if (!memcmp(p, ff_asf_stream_header,
> sizeof(ff_asf_guid))) {
> > +            flags     = AV_RL16(p + sizeof(ff_asf_guid)*3 + 24);
> > +            stream_id = flags & 0x7F;
> > +            //The second condition is for checking
> CS_PKT_STREAM_ID_REQUEST packet size,
> > +            //we can calcuate the packet size by stream_num.
> > +            //Please see function send_stream_selection_request().
> > +            if (mms->stream_num < MAX_STREAMS &&
> > +                    46 + mms->stream_num * 6 < sizeof(mms->out_buffer))
> {
> > +                mms->streams[mms->stream_num].id = stream_id;
> > +                mms->stream_num++;
> > +            } else
> > +                dprintf("Too many streams.\n");
> > +        }
> > +        if (chunksize > end - p)
> > +            return -1;
> > +        p += chunksize;
> > +    } while (end - p >= sizeof(ff_asf_guid) + 8);
>
> this can end in an infinite loop
>
>
>
sharp vision!
yes, if chunksize=0, the loop will be dead loop.

zhentan
-- 
Best wishes~



More information about the ffmpeg-devel mailing list