[FFmpeg-devel] [patch]MMS protocol over TCP
Michael Niedermayer
michaelni
Tue Mar 30 23:23:10 CEST 2010
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
>
>
> >
> > > + memcpy(mms->asf_header +
> > mms->asf_header_size,
> > > + mms->pkt_read_ptr,
> > > + mms->pkt_buf_len);
> > > + mms->asf_header_size += mms->pkt_buf_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);
> > > + done= 0;
> > > + }
> > > + }
> > > + }
> > > + } 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;
> > > + }
> >
> > > + done = 1;
> >
> > can done be anything else than 1 here?
> >
> >
> yes, it's the end of the loop.
> we retrieve data less than 8 bytes, it must be an error.
> so we can return it directly at here.
>
> >
> > [...]
> > > +static int asf_header_parser(MMSContext *mms)
> > > +{
> > > + uint8_t *p = mms->asf_header, *end = mms->asf_header +
> > mms->asf_header_size;
> > > + 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);
> > > + }
> > > + } else if (!memcmp(p, ff_asf_stream_header,
> > sizeof(ff_asf_guid))) {
> > > + mms->stream_num++;
> >
> > shouldnt the number be limited somehow? at some place in the code
> > you write data for each stream into a fixed size buffer
> >
> > what do you mean about this?
> could you explain it more?
> thanks
it seems to me that above code can increase stream_num without bound.
I saw no checks for stream_num becoming too large.
but there is code which for example stores stream_num as a 16bit
value, this would fail if stream_num become greater than 65536.
Also there is IIRC some code writing values for each stream thus
stream_num values into some buffer of fixed size.
[...]
> >
> > [...]
> > > + mms = av_malloc(sizeof(MMSContext));
> > > + if (!mms)
> > > + return AVERROR(ENOMEM);
> > > + memset(mms, 0, sizeof(MMSContext));
> >
> > av_mallocz()
> >
> >
> > [...]
> > > +/** Read ASF data through the protocol. */
> > > +static int mms_read(URLContext *h, uint8_t *buf, int size)
> > > +{
> > > + /* TODO: see tcp.c:tcp_read() about a possible timeout scheme */
> > > + MMSContext *mms = h->priv_data;
> > > + int result = 0;
> > > +
> > > + /* Since we read the header at open(), this shouldn't be possible */
> > > + assert(mms->header_parsed);
> > > +
> > > + if (mms->asf_header_read_pos >= mms->asf_header_size
> > > + && !mms->streaming_flag) {
> > > + dprintf(NULL, "mms_read() before play().\n");
> > > + clear_stream_buffers(mms);
> > > + result = send_stream_selection_request(mms);
> > > + if(result < 0)
> > > + return result;
> > > + if (get_tcp_server_response(mms) != SC_PKT_STREAM_ID_ACCEPTED) {
> > > + dprintf(NULL, "Can't get stream id accepted packet.\n");
> > > + return 0;
> > > + }
> > > +
> > > + // send media packet request
> > > + send_media_packet_request(mms);
> > > + if (get_tcp_server_response(mms) == SC_PKT_MEDIA_PKT_FOLLOWS) {
> > > + mms->streaming_flag = 1;
> > > + } else {
> >
> > > + dprintf(NULL, "Canot get media follows packet from
> > server.\n");
> >
> > using NULL as context for dprintf() is not ideal
> >
> >
> yes.
> but if I use pctx instead of NULL, I should add param URLContext *h for
> every function which call dprintf().
> the added param will only be used for print out the log info.
> Is it really necessary?
no, i thought it was easy.
Please replace NULL by URLContext where URLContext is alraedy available.
You can keep using NULL for dprintf() where URLContext is not available
[...]
--
Michael GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB
Avoid a single point of failure, be that a person or equipment.
-------------- 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/20100330/d10fbd3e/attachment.pgp>
More information about the ffmpeg-devel
mailing list