[FFmpeg-devel] [patch]add mmsh protocol and extract common code for mmst.c

[zhentan feng]

Hi

On Fri, Aug 13, 2010 at 6:00 AM, Ronald S. Bultje <rsbultje at gmail.com>wrote:

> Hi,
>
> On Thu, Aug 12, 2010 at 1:26 PM, zhentan feng <spyfeng at gmail.com> wrote:
> > On Thu, Aug 12, 2010 at 7:14 AM, Ronald S. Bultje <rsbultje at gmail.com
> >wrote:
> >> On Mon, Aug 9, 2010 at 12:05 PM, zhentan feng <spyfeng at gmail.com>
> wrote:
> >> > #9 adds mmsh.c
> >>
> >>
> >> > +#define CHUNK_TYPE_DATA           0x4424
> >> > +#define CHUNK_TYPE_ASF_HEADER     0x4824
> >> > +#define CHUNK_TYPE_END            0x4524
> >> > +#define CHUNK_TYPE_STREAM_CHANGE  0x4324
> >>
> >> Do these mean anything? (If not, that's OK, just wondering...)
> >>
> >> You could consider making CHUNK_TYPE_* an enum.
> >
> > the value has special meaning. I add comment for this.
>
> You can still make them an enum:
>
> enum {
>    CHUNK_TYPE_DATA = 0x4424,
> [etc]
> };
>
> That way the doxy output is nicer. You can also add some doxy to say
> what a chunk_type is or so and it'll group them.
>
> > I have fixed the code according to each reviewing item.
> > please see the new patch for mmsh.c
> [..]
> > +// see Ref 2.2.3 for packet type define
> > +#define CHUNK_TYPE_DATA           0x4424
> > +#define CHUNK_TYPE_ASF_HEADER     0x4824
> > +#define CHUNK_TYPE_END            0x4524
> > +#define CHUNK_TYPE_STREAM_CHANGE  0x4324
>
> Please make this an enum, as mentioned above.
>
> > +// mmsh request messages.
> > +static const char* mmsh_first_request =
> > +    "Accept: */*\r\n"
> > +    USERAGENT
> > +    "Host: %s:%d\r\n"
> > +    "Pragma:
> no-cache,rate=1.000000,stream-time=0,stream-offset=0:0,request-context=%u,max-duration=0\r\n"
> > +    CLIENTGUID
> > +    "Connection: Close\r\n\r\n";
> > +
> > +static const char* mmsh_live_request =
> > +    "Accept: */*\r\n"
> > +    USERAGENT
> > +    "Host: %s:%d\r\n"
> > +    "Pragma: no-cache,rate=1.000000,request-context=%u\r\n"
> > +    "Pragma: xPlayStrm=1\r\n"
> > +    CLIENTGUID
> > +    "Pragma: stream-switch-count=%d\r\n"
> > +    "Pragma: stream-switch-entry=%s\r\n"
> > +    "Connection: Close\r\n\r\n";
>
> As I mentioned before, you can inline those in the code where they're
> used, since each is used only once.
>
>

oh, yes. I forgot this one.

> > +static int get_chunk_header(MMSHContext *mmsh, int *len)
> [..]
> > +    switch (chunk_type) {
> > +    case CHUNK_TYPE_END:
> > +    case CHUNK_TYPE_STREAM_CHANGE:
> > +        ext_header_len = 4;
> > +        break;
> > +    case CHUNK_TYPE_ASF_HEADER:
> > +    case CHUNK_TYPE_DATA:
> > +        ext_header_len = 8;
> > +        break;
> > +    default:
> > +        av_log(NULL, AV_LOG_ERROR, "strange chunk type %d\n",
> chunk_type);
> > +        return AVERROR_INVALIDDATA;
> > +    }
> > +
> > +    if (ext_header_len > EXT_HEADER_LENGTH) {
> > +        av_log(NULL, AV_LOG_ERROR, "ext_header_len = %d exceed the
> buffer size %d\n",
> > +                    ext_header_len, EXT_HEADER_LENGTH);
> > +        return AVERROR_INVALIDDATA;
> > +    }
>
> That can never happen, can it?
>
>

hmm, yes. I was hesitating to add this.
When I thought if ext_header_len is artificial value for malicious.
overwrite could happen.

> > +static int read_data_packet(MMSHContext *mmsh, const int len)
> [..]
> > +    int res, pad_size = 0;
> [..]
> > +    } else {
> > +        pad_size = mms->asf_packet_len - len;
> > +        memset(mms->in_buffer + len, 0, pad_size);
> > +    }
>
> Only used once and in local context, so you can remove pad_size as a
> variable.
>
> > +static int get_http_header_data(MMSHContext *mmsh)
> [..]
> > +            if (res != len) {
> > +                av_log(NULL, AV_LOG_ERROR, "recv asf header data len %d
> != %d", res, len);
>
> Missing \n.
>
> > +        } else {
> > +            if (len) {
> > +                if (len > sizeof(mms->in_buffer)) {
> > +                    av_log(NULL, AV_LOG_ERROR, "other packet len = %d
> exceed the in_buffer size %d\n",
> > +                                len, sizeof(mms->in_buffer));
> > +                    return AVERROR_IO;
> > +                }
> > +                res = url_read_complete(mms->mms_hd, mms->in_buffer,
> len);
> > +                if (res != len) {
> > +                    av_log(NULL, AV_LOG_ERROR, "read other chunk type
> data failed!\n");
> > +                    return AVERROR(EIO);
> > +                } else {
> > +                    dprintf(NULL, "skip chunk type %d \n", chunk_type);
> > +                    continue;
> > +                }
> > +            }
> > +        }
>
> Did you try using url_fskip() here, as suggested earlier?
>
>

I am sorry. I didn't try it.
But I was considering that if use url_fskip(ByteIOContext *s, int64_t
offset), we should introduce ByteIOContext  struct.
we did some work for removing ByteIOContext  in mmst.c so IMHO it 's not
should be used here.

thanks your reviewing works.
zhentan
-- 
Best wishes~