[FFmpeg-devel] [patch]MMS protocol over TCP
Michael Niedermayer
michaelni
Wed May 19 01:48:24 CEST 2010
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!
> + }
> + }
> + } 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
[...]
> + if (err)
> + goto fail;
> + else {
nitpick: this should have a {}
[...]
> + if (result)
> + return result;
> + else
{}
[...]
--
Michael GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB
I wish the Xiph folks would stop pretending they've got something they
do not. Somehow I fear this will remain a wish. -- M?ns Rullg?rd
-------------- 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/20100519/b999ef71/attachment.pgp>
More information about the ffmpeg-devel
mailing list