[FFmpeg-devel] [patch]MMS protocol over TCP
zhentan feng
spyfeng
Tue Apr 20 17:58:53 CEST 2010
Hi
On Tue, Apr 20, 2010 at 9:09 PM, Michael Niedermayer <michaelni at gmx.at>wrote:
> On Tue, Apr 20, 2010 at 12:46:17AM +0800, zhentan feng wrote:
> > Hi
> >
> > On Mon, Apr 19, 2010 at 6:39 AM, Michael Niedermayer <michaelni at gmx.at
> >wrote:
> [...]
> > >
> > > [...]
> > > > +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);
> > >
> > > this looks unsafe, there is code that writes based on asf_packet_len
> >
> >
> > how the code make it unsafe? could you please explain it?
>
> here:
> > + int padding_size = mms->asf_packet_len - mms->pkt_buf_len;
> > + memset(mms->incoming_buffer + mms->pkt_buf_len, 0,
> padding_size);
>
> the memset maybe can write over the end of the array
>
fixed like this:
--- mms/mmst.c Mon Apr 19 18:35:30 2010 (r5764)
+++ mms/mmst.c Tue Apr 20 17:04:45 2010 (r5765)
@@ -411,6 +411,10 @@ static int asf_header_parser(MMSContext
/* 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 > sizeof(mms->incoming_buffer)) {
+ dprintf(NULL,"Too large packet len:%d"
+ " may overwrite incoming_buffer when padding",
mms->asf_packet_len);
+ }
}
} else if (!memcmp(p, ff_asf_stream_header, sizeof(ff_asf_guid))) {
flags = AV_RL16(p + sizeof(ff_asf_guid)*3 + 24);
> [...]
> > here is the new version patch attached below.
> > please review.
> > zhentan
> > --
> > Best wishes~
>
> > Makefile | 1
> > allformats.c | 1
> > mmst.c | 679
> +++++++++++++++++++++++++++++++++++++++++++++++++++++++++++
> > 3 files changed, 681 insertions(+)
> > ca0c1492faa9d4343af8882ec02392ccfe4c7851 mmst_10.patch
> > Index: libavformat/mmst.c
> > ===================================================================
> > --- libavformat/mmst.c (revision 0)
> > +++ libavformat/mmst.c (revision 0)
> > @@ -0,0 +1,679 @@
> > +/*
> > + * MMS protocol over TCP
> > + * Copyright (c) 2006,2007 Ryan Martell
> > + * Copyright (c) 2007 Bj?rn Axelsson
> > + * Copyright (c) 2010 Zhentan Feng <spyfeng at gmail dot com>
> > + *
> > + * This file is part of FFmpeg.
> > + *
> > + * FFmpeg is free software; you can redistribute it and/or
> > + * modify it under the terms of the GNU Lesser General Public
> > + * License as published by the Free Software Foundation; either
> > + * version 2.1 of the License, or (at your option) any later version.
> > + *
> > + * FFmpeg is distributed in the hope that it will be useful,
> > + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> > + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU
> > + * Lesser General Public License for more details.
> > + *
> > + * You should have received a copy of the GNU Lesser General Public
> > + * License along with FFmpeg; if not, write to the Free Software
> > + * Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston, MA
> 02110-1301 USA
> > + */
> > +#include "avformat.h"
> > +#include "internal.h"
> > +#include "libavutil/intreadwrite.h"
> > +#include "libavcodec/bytestream.h"
> > +#include "network.h"
> > +#include "asf.h"
> > +
> > +#define LOCAL_ADDRESS 0xc0a80081 // FIXME get and use correct local
> ip address.
> > +#define LOCAL_PORT 1037 // as above.
> > +/** Client to server packet types. */
> > +typedef enum {
> > + CS_PKT_INITIAL = 0x01,
> > + CS_PKT_PROTOCOL_SELECT = 0x02,
> > + CS_PKT_MEDIA_FILE_REQUEST = 0x05,
> > + CS_PKT_START_FROM_PKT_ID = 0x07,
> > + CS_PKT_STREAM_PAUSE = 0x09,
> > + CS_PKT_STREAM_CLOSE = 0x0d,
> > + CS_PKT_MEDIA_HEADER_REQUEST = 0x15,
> > + CS_PKT_TIMING_DATA_REQUEST = 0x18,
> > + CS_PKT_USER_PASSWORD = 0x1a,
> > + CS_PKT_KEEPALIVE = 0x1b,
> > + CS_PKT_STREAM_ID_REQUEST = 0x33,
> > +} MMSCSPacketType;
> > +
> > +/** Server to client packet types. */
> > +typedef enum {
> > + /** Control packets. */
> > + /*@{*/
> > + SC_PKT_CLIENT_ACCEPTED = 0x01,
> > + SC_PKT_PROTOCOL_ACCEPTED = 0x02,
> > + SC_PKT_PROTOCOL_FAILED = 0x03,
> > + SC_PKT_MEDIA_PKT_FOLLOWS = 0x05,
> > + SC_PKT_MEDIA_FILE_DETAILS = 0x06,
> > + SC_PKT_HEADER_REQUEST_ACCEPTED = 0x11,
> > + SC_PKT_TIMING_TEST_REPLY = 0x15,
> > + SC_PKT_PASSWORD_REQUIRED = 0x1a,
> > + SC_PKT_KEEPALIVE = 0x1b,
> > + SC_PKT_STREAM_STOPPED = 0x1e,
> > + SC_PKT_STREAM_CHANGING = 0x20,
> > + SC_PKT_STREAM_ID_ACCEPTED = 0x21,
> > + /*@}*/
> > +
> > + /** Pseudo packets. */
> > + /*@{*/
> > + SC_PKT_CANCEL = -1,
> > + SC_PKT_NO_DATA = -2,
> > + SC_PKT_HTTP_CONTROL_ACKNOWLEDGE = -3,
> > + /*@}*/
> > +
> > + /** Data packets. */
> > + /*@{*/
> > + SC_PKT_ASF_HEADER = 0x81,
> > + SC_PKT_ASF_MEDIA = 0x82,
> > + /*@}*/
> > +} MMSSCPacketType;
> > +
> > +typedef struct {
> > + int id;
> > +}MMSStream;
> > +
> > +typedef struct {
> > + int outgoing_packet_seq; ///< Outgoing packet sequence
> number.
> > + char path[256]; ///< Path of the resource being
> asked for.
> > + char host[128]; ///< Host of the resources.
> > +
> > + URLContext *mms_hd; ///< TCP connection handle
> > + MMSStream streams[MAX_STREAMS];
> > +
> > + /** Buffer for outgoing packets. */
>
> > + /*@{*/
> > + uint8_t *write_ptr; ///< Pointer for writting the
> buffer.
> > + uint8_t outgoing_packet_buffer[512]; ///< Buffer for outgoing
> packet.
> > + /*@}*/
> > +
> > + /** Buffer for incoming packets. */
> > + /*@{*/
> > + uint8_t incoming_buffer[8192]; ///< Buffer for incoming
> packets.
> > + uint8_t *pkt_read_ptr; ///< Pointer for reading from
> incoming buffer.
> > + int pkt_buf_len; ///< Reading length from
> incoming buffer.
> > + /*@}*/
>
> i would suggest more consistentnames like for example:
> uint8_t out_buffer[512];
> uint8_t *write_out_ptr;
>
> uint8_t in_buffer[8192]
> uint8_t *read_in_ptr;
> int remaining_in_length;
>
> (of course you can use different names if you prefer)
>
>
renamed them.
>
>
> [...]
> > +/** 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->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.
> > + if (length_remaining < 0
> > + || length_remaining >
> sizeof(mms->incoming_buffer) - 12) {
> > + dprintf("Incoming message len %d exceeds buffer
> len %d\n",
> > + length_remaining,
> sizeof(mms->incoming_buffer) - 12);
> > + break;
> > + }
> > + read_result = url_read_complete(mms->mms_hd,
> mms->incoming_buffer + 12,
> > + length_remaining) ;
> > + if (read_result == length_remaining) {
>
> > + packet_type= AV_RL16(mms->incoming_buffer+36);
>
> is any value valid here?
> i think this is missing some checks
>
> packet_type is handled later with exception checks.
I think it's not necessary to check it here.
>
> > +
> > + } 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
>
> this comment does not seem correct anymore
>
removed.
>
> [...]
> > +/** Send MMST stream selection command based on the AVStream->discard
> values. */
> > +static int send_stream_selection_request(MMSContext *mms)
> > +{
> > + int i;
> > +
> > + // send the streams we want back...
> > + start_command_packet(mms, CS_PKT_STREAM_ID_REQUEST);
> > + bytestream_put_le32(&mms->write_ptr, mms->stream_num); //
> stream nums
> > + if (mms->write_ptr - mms->outgoing_packet_buffer >
> > + sizeof(mms->outgoing_packet_buffer) - 6 * mms->stream_num) {
> > + dprintf("buffer will overflow for too many streams: %d.\n",
> mms->stream_num);
> > + return AVERROR_IO;
> > + }
>
> this should probably be checked before adding the streams
>
> fixed it like this:
--- mms/mmst.c Tue Apr 20 17:39:15 2010 (r5772)
+++ mms/mmst.c Tue Apr 20 17:50:20 2010 (r5773)
@@ -417,7 +417,8 @@ static int asf_header_parser(MMSContext
} 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;
- if (mms->stream_num < MAX_STREAMS) {
+ 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
@@ -439,11 +440,6 @@ static int send_stream_selection_request
// send the streams we want back...
start_command_packet(mms, CS_PKT_STREAM_ID_REQUEST);
bytestream_put_le32(&mms->write_out_ptr, mms->stream_num); //
stream nums
- if (mms->write_out_ptr - mms->out_buffer >
- sizeof(mms->out_buffer) - 6 * mms->stream_num) {
- dprintf("buffer will overflow for too many streams: %d.\n",
mms->stream_num);
- return AVERROR_IO;
- }
for(i= 0; i<mms->stream_num; i++) {
bytestream_put_le16(&mms->write_out_ptr, 0xffff); //
flags
bytestream_put_le16(&mms->write_out_ptr, mms->streams[i].id); //
stream id
[...]
new version patched attached below.
zhentan
--
Best wishes~
-------------- next part --------------
A non-text attachment was scrubbed...
Name: mmst_11.patch
Type: application/octet-stream
Size: 26584 bytes
Desc: not available
URL: <http://lists.mplayerhq.hu/pipermail/ffmpeg-devel/attachments/20100420/4a8847a3/attachment.obj>
More information about the ffmpeg-devel
mailing list