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.
> int is_multi_client;
> HandshakeState handshake_step;
> int is_connected_server;
> + int skip_read_header;
This one belongs in another patch.
> } 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/
> 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.
> 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.
> }
> }
> return 1;
Regards,
--
Nicolas George
signature.asc
Description: Digital signature
_______________________________________________ ffmpeg-devel mailing list ffmpeg-devel@ffmpeg.org http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
