On Thu, Aug 20, 2015 at 11:02 AM, Nicolas George <geo...@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 <klaxa1...@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@ffmpeg.org
> http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
>
Regards,
Stephan
_______________________________________________
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
