[FFmpeg-devel] [PATCH 1/2] lavf/http: Parse and/or expose various client data.

Stephan Holljes klaxa1337 at googlemail.com
Thu Aug 20 17:09:51 CEST 2015


On Thu, Aug 20, 2015 at 11:02 AM, Nicolas George <george at nsup.org> wrote:
> Thanks for the patch. A few remarks, specific to the stand-alone-commit
> version or that I had not yet spotted.
>
>
> Le tridi 3 fructidor, an CCXXIII, Stephan Holljes a écrit :
>> Signed-off-by: Stephan Holljes <klaxa1337 at googlemail.com>
>> ---
>>  libavformat/http.c | 40 ++++++++++++++++++++++++++++++++++++++--
>>  1 file changed, 38 insertions(+), 2 deletions(-)
>
> Maybe list the features in the details of the commit message.
>
>>
>> diff --git a/libavformat/http.c b/libavformat/http.c
>> index a136918..bfe6801 100644
>> --- a/libavformat/http.c
>> +++ b/libavformat/http.c
>> @@ -64,12 +64,14 @@ typedef struct HTTPContext {
>>      int64_t chunksize;
>>      int64_t off, end_off, filesize;
>>      char *location;
>
>> +    char *host;
>>      HTTPAuthState auth_state;
>>      HTTPAuthState proxy_auth_state;
>>      char *headers;
>>      char *mime_type;
>>      char *user_agent;
>>      char *content_type;
>> +    char *body;
>>      /* Set if the server correctly handles Connection: close and will close
>>       * the connection after feeding us the content. */
>>      int willclose;
>> @@ -107,10 +109,14 @@ typedef struct HTTPContext {
>>      int reconnect;
>>      int listen;
>>      char *resource;
>> +    char *query;
>> +    char *http_version;
>>      int reply_code;
>> +    char *reply_text;
>
> Do you have a system to choose where you add each field? If not, maybe it
> would be a good idea to group thematically: everything that is for server
> only together, with "strings parsed from the header" even more together.

My system has been to put things that seem related together. I will
think of a logical order and create a different patch that reorders
the fields.

>
>>      int is_multi_client;
>>      HandshakeState handshake_step;
>>      int is_connected_server;
>
>> +    int skip_read_header;
>
> This one belongs in another patch.

Ah, of course.

>
>>  } HTTPContext;
>>
>>  #define OFFSET(x) offsetof(HTTPContext, x)
>> @@ -123,6 +129,7 @@ static const AVOption options[] = {
>>      { "chunked_post", "use(s) chunked transfer-encoding for posts", OFFSET(chunked_post), AV_OPT_TYPE_INT, { .i64 = 1 }, 0, 1, D | E },
>>      { "headers", "set custom HTTP headers, can override built in default headers", OFFSET(headers), AV_OPT_TYPE_STRING, { 0 }, 0, 0, D | E },
>>      { "content_type", "set a specific content type for the POST messages", OFFSET(content_type), AV_OPT_TYPE_STRING, { 0 }, 0, 0, D | E },
>> +    { "body", "set the body of a simple HTTP reply", OFFSET(body), AV_OPT_TYPE_STRING, { .str = NULL }, 0, 0, E },
>>      { "user_agent", "override User-Agent header", OFFSET(user_agent), AV_OPT_TYPE_STRING, { .str = DEFAULT_USER_AGENT }, 0, 0, D },
>>      { "user-agent", "override User-Agent header", OFFSET(user_agent), AV_OPT_TYPE_STRING, { .str = DEFAULT_USER_AGENT }, 0, 0, D },
>>      { "multiple_requests", "use persistent connections", OFFSET(multiple_requests), AV_OPT_TYPE_INT, { .i64 = 0 }, 0, 1, D | E },
>> @@ -141,10 +148,14 @@ static const AVOption options[] = {
>>      { "offset", "initial byte offset", OFFSET(off), AV_OPT_TYPE_INT64, { .i64 = 0 }, 0, INT64_MAX, D },
>>      { "end_offset", "try to limit the request to bytes preceding this offset", OFFSET(end_off), AV_OPT_TYPE_INT64, { .i64 = 0 }, 0, INT64_MAX, D },
>>      { "method", "Override the HTTP method or set the expected HTTP method from a client", OFFSET(method), AV_OPT_TYPE_STRING, { .str = NULL }, 0, 0, D | E },
>> +    { "http_version", "The http version string received from a client.", OFFSET(http_version), AV_OPT_TYPE_STRING, {.str = NULL}, 0, 0, D | AV_OPT_FLAG_READONLY },
>>      { "reconnect", "auto reconnect after disconnect before EOF", OFFSET(reconnect), AV_OPT_TYPE_INT, { .i64 = 0 }, 0, 1, D },
>>      { "listen", "listen on HTTP", OFFSET(listen), AV_OPT_TYPE_INT, { .i64 = 0 }, 0, 2, D | E },
>>      { "resource", "The resource requested by a client", OFFSET(resource), AV_OPT_TYPE_STRING, { .str = NULL }, 0, 0, E },
>> +    { "query", "The query requested by a client", OFFSET(query), AV_OPT_TYPE_STRING, { .str = NULL }, 0, 0, E },
>>      { "reply_code", "The http status code to return to a client", OFFSET(reply_code), AV_OPT_TYPE_INT, { .i64 = 200}, INT_MIN, 599, E},
>> +    { "reply_text", "Set a custom reply text to replace standard HTTP replies", OFFSET(reply_text), AV_OPT_TYPE_STRING, { .str = NULL}, 0, 0, E},
>> +    { "hostname", "The Host-parameter of an incoming HTTP request.", OFFSET(host), AV_OPT_TYPE_STRING, { .str = NULL}, 0, 0, E},
>>      { NULL }
>>  };
>>
>> @@ -506,6 +517,7 @@ static int http_accept(URLContext *s, URLContext **c)
>>      HTTPContext *cc;
>>      URLContext *sl = sc->hd;
>>      URLContext *cl = NULL;
>> +    char *peeraddr;
>>
>>      av_assert0(sc->listen);
>>      if ((ret = ffurl_alloc(c, s->filename, s->flags, &sl->interrupt_callback)) < 0)
>> @@ -515,6 +527,9 @@ static int http_accept(URLContext *s, URLContext **c)
>>          goto fail;
>>      cc->hd = cl;
>>      cc->is_multi_client = 1;
>
>> +    if ((ret = av_opt_get(cc, "sock_addr_str", AV_OPT_SEARCH_CHILDREN, &peeraddr)) < 0)
>> +        return ret;
>> +    av_log(s, AV_LOG_TRACE, "Peer address: %s\n", peeraddr);
>
> av_opt_get() strdup()s the option, so it must be freed(). But maybe leaving
> an expensive operation just for debug is not necessary/

Ok removed. I think I used it to check that nested AVOptions worked.

>
>>  fail:
>>      return ret;
>>  }
>> @@ -702,7 +717,7 @@ static int process_line(URLContext *h, char *line, int line_count,
>>  {
>>      HTTPContext *s = h->priv_data;
>>      const char *auto_method =  h->flags & AVIO_FLAG_READ ? "POST" : "GET";
>> -    char *tag, *p, *end, *method, *resource, *version;
>> +    char *tag, *p, *end, *method, *resource, *query = NULL, *version;
>>      int ret;
>>
>>      /* end of header */
>> @@ -720,6 +735,7 @@ static int process_line(URLContext *h, char *line, int line_count,
>>                  p++;
>>              *(p++) = '\0';
>>              av_log(h, AV_LOG_TRACE, "Received method: %s\n", method);
>> +            av_log(h, AV_LOG_TRACE, "Expected method: %s\n", s->method);
>>              if (s->method) {
>>                  if (av_strcasecmp(s->method, method)) {
>>                      av_log(h, AV_LOG_ERROR, "Received and expected HTTP method do not match. (%s expected, %s received)\n",
>> @@ -742,12 +758,22 @@ static int process_line(URLContext *h, char *line, int line_count,
>>              while (av_isspace(*p))
>>                  p++;
>>              resource = p;
>> -            while (!av_isspace(*p))
>
>> +            while (!av_isspace(*p) && *p != '?')
>
> This test accepts 0, making it read beyond the allocated string on short
> requests. This was already present in the original code, sorry for not
> spotting it when the first patch was applied. There is a similar issue below
> IIRC.
>
> It is easy fixed: add "*p &&" at the beginning of the test. And it is rather
> urgent, since there is a small chance it can be exploited. Fortunately,
> nobody is probably using the feature for now.

Added to all while(!av_isspace(p)) loops in the server code.

>
>>                  p++;
>> +            if (*p == '?') {
>> +                *(p++) = '\0';
>> +                query = p;
>> +                while (!av_isspace(*p))
>> +                    p++;
>> +                if (!(s->query = av_strdup(query)))
>> +                    return AVERROR(ENOMEM);
>> +            }
>>              *(p++) = '\0';
>>              av_log(h, AV_LOG_TRACE, "Requested resource: %s\n", resource);
>>              if (!(s->resource = av_strdup(resource)))
>>                  return AVERROR(ENOMEM);
>> +            if (query)
>> +                av_log(h, AV_LOG_TRACE, "Requested query: %s\n", query);
>>
>>              // HTTP version
>>              while (av_isspace(*p))
>> @@ -761,6 +787,8 @@ static int process_line(URLContext *h, char *line, int line_count,
>>                  return ff_http_averror(400, AVERROR(EIO));
>>              }
>>              av_log(h, AV_LOG_TRACE, "HTTP version string: %s\n", version);
>> +            if (!(s->http_version = av_strdup(version)))
>> +                return AVERROR(ENOMEM);
>>          } else {
>>              while (!av_isspace(*p) && *p != '\0')
>>                  p++;
>> @@ -831,6 +859,14 @@ static int process_line(URLContext *h, char *line, int line_count,
>>          } else if (!av_strcasecmp(tag, "Content-Encoding")) {
>>              if ((ret = parse_content_encoding(h, p)) < 0)
>>                  return ret;
>> +        } else if (s->is_connected_server && !av_strcasecmp(tag, "Host")) {
>> +            av_log(s, AV_LOG_TRACE, "Host: %s\n", p);
>> +            if (!s->host && !(s->host = av_strdup(p)))
>> +                return AVERROR(ENOMEM);
>
>> +        } else if (s->is_connected_server && !av_strcasecmp(tag, "User-Agent")) {
>> +            av_log(s, AV_LOG_TRACE, "User-Agent: %s\n", p);
>> +            if (!s->user_agent && !(s->user_agent = av_strdup(p)))
>> +                return AVERROR(ENOMEM);
>
> I would say that the user-agent header does not deserve a special treatment
> at the protocol level, but if it is convenient for applications (a lot of
> servers log it), it is a matter of taste.

This is more or less temporal, since the user-agent field is/will be
used by ffserver.
Once headers are handled by an AVDictionary this code will probably be removed.

>
>>          }
>>      }
>>      return 1;
>
> Regards,
>
> --
>   Nicolas George
>
> _______________________________________________
> ffmpeg-devel mailing list
> ffmpeg-devel at ffmpeg.org
> http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
>

Regards,
Stephan


More information about the ffmpeg-devel mailing list