[FFmpeg-devel] [PATCH][GSoC] lavf/http: Parse and set HTTP method when listening on HTTP(S)
Nicolas George
george at nsup.org
Wed Jun 3 22:39:58 CEST 2015
Le quintidi 15 prairial, an CCXXIII, Stephan Holljes a écrit :
> I also just now noticed while testing that sending a CTRL+C interrupt
> results in a segmentation fault, because no client was connected and
> ffurl_write() tried to send data anyway. That is (hopefully) fixed
> now.
Good to have caught it.
> > And it would probably be a good idea to isolate that part in a separate
> > function.
> Can you point me to a function where I can see how this should be done
> properly? I've been thinking for a long time and I couldn't come up
> with a clean solution.
http_listen(), you wrote it yourself. Maybe I was not clear enough, I just
suggested to move the code in a function for better readability (the name of
the function is self-documentating), nothing more complicated.
> Agreed, if unset the method is ignored for now. In the future
> auto-detection should be used.
Seems ok.
> Also, the complete first line is now parsed for method, resource and
> HTTP version.
Good.
> From 3af5c5867672624d45de447d11c2107b3c77742c Mon Sep 17 00:00:00 2001
> From: Stephan Holljes <klaxa1337 at googlemail.com>
> Date: Wed, 3 Jun 2015 20:57:53 +0200
> Subject: [PATCH 5/5] lavf/http: Indent else-clause.
>
> Signed-off-by: Stephan Holljes <klaxa1337 at googlemail.com>
> ---
> libavformat/http.c | 16 ++++++++--------
> 1 file changed, 8 insertions(+), 8 deletions(-)
This looks ok.
Personally, I have the habit of inserting "/* TODO reindent */" in the code
where it is needed and only make the reindent patch at the last moment, to
reduce rebase nuisance.
> From 90f17ce9711304228bc3b7bf8cb4cdfac6b14011 Mon Sep 17 00:00:00 2001
> From: Stephan Holljes <klaxa1337 at googlemail.com>
> Date: Wed, 3 Jun 2015 20:57:23 +0200
> Subject: [PATCH 4/5] lavf/http: Properly process HTTP header on listen.
>
> Signed-off-by: Stephan Holljes <klaxa1337 at googlemail.com>
> ---
> libavformat/http.c | 44 +++++++++++++++++++++++++++++++++++++++++++-
> 1 file changed, 43 insertions(+), 1 deletion(-)
>
> diff --git a/libavformat/http.c b/libavformat/http.c
> index 6fad584..dfe2616 100644
> --- a/libavformat/http.c
> +++ b/libavformat/http.c
> @@ -558,7 +558,7 @@ static int process_line(URLContext *h, char *line, int line_count,
> int *new_location)
> {
> HTTPContext *s = h->priv_data;
> - char *tag, *p, *end;
> + char *tag, *p, *end, *method, *resource, *version;
> int ret;
>
> /* end of header */
> @@ -569,6 +569,47 @@ static int process_line(URLContext *h, char *line, int line_count,
>
> p = line;
> if (line_count == 0) {
> + if (s->listen) {
> + // HTTP method
> + while (av_isspace(*p))
> + p++;
I believe spaces are not allowed before the request.
> + method = p;
> + while (!av_isspace(*p))
> + p++;
> + *p = '\0';
Plain 0 is acceptable, your choice.
> + av_log(h, AV_LOG_TRACE, "Received method: %s\n", 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",
> + s->method, method);
> + return ff_http_averror(400, AVERROR(EIO));
> + }
> + }
> + p++;
Maybe better keep it grouped with "*p = 0", possibly in a single step:
*(p++) = 0.
> +
> + // HTTP resource
> + while (av_isspace(*p))
> + p++;
> + resource = p;
> + while (!av_isspace(*p))
> + p++;
> + *p = '\0';
> + p++;
> + av_log(h, AV_LOG_TRACE, "Requested resource: %s\n", resource);
> +
> + // HTTP version
> + while (av_isspace(*p))
> + p++;
> + version = p;
> + while (!av_isspace(*p))
> + p++;
> + *p = '\0';
> + if (av_strncasecmp(version, "HTTP/", 5)) {
> + av_log(h, AV_LOG_ERROR, "Malformed HTTP version string.\n");
> + return ff_http_averror(400, AVERROR(EIO));
> + }
> + av_log(h, AV_LOG_TRACE, "HTTP version string: %s\n", version);
> + } else {
> while (!av_isspace(*p) && *p != '\0')
> p++;
> while (av_isspace(*p))
> @@ -579,6 +620,7 @@ static int process_line(URLContext *h, char *line, int line_count,
>
> if ((ret = check_http_code(h, s->http_code, end)) < 0)
> return ret;
> + }
> } else {
> while (*p != '\0' && *p != ':')
> p++;
> --
> 2.1.0
>
> From bbe015d20c1852e7c0c8885ca49a8ba2d926911d Mon Sep 17 00:00:00 2001
> From: Stephan Holljes <klaxa1337 at googlemail.com>
> Date: Wed, 3 Jun 2015 20:45:08 +0200
> Subject: [PATCH 3/5] lavf/http: Rudimentary error handling for HTTP requests
> received from clients.
>
> Signed-off-by: Stephan Holljes <klaxa1337 at googlemail.com>
> ---
> libavformat/http.c | 13 +++++++++++++
> 1 file changed, 13 insertions(+)
>
> diff --git a/libavformat/http.c b/libavformat/http.c
> index e51f524..6fad584 100644
> --- a/libavformat/http.c
> +++ b/libavformat/http.c
> @@ -304,6 +304,9 @@ static int http_listen(URLContext *h, const char *uri, int flags,
> HTTPContext *s = h->priv_data;
> int ret;
> static const char header[] = "HTTP/1.1 200 OK\r\nContent-Type: application/octet-stream\r\nTransfer-Encoding: chunked\r\n\r\n";
> + static const char bad_request[] = "HTTP/1.1 400 Bad Request\r\nContent-Type: text/plain\r\n\r\n400 Bad Request\r\n";
> + static const char internal_server_error[] = "HTTP/1.1 500 Internal server error\r\nContent-Type: text/plain\r\n\r\n500 Internal server error\r\n";
> +
> char hostname[1024], proto[10];
> char lower_url[100];
> const char *lower_proto = "tcp";
> @@ -325,6 +328,16 @@ static int http_listen(URLContext *h, const char *uri, int flags,
> return 0;
>
> fail:
> + if (h->is_connected) {
> + switch(ret) {
> + case AVERROR_HTTP_BAD_REQUEST:
> + ffurl_write(s->hd, bad_request, strlen(bad_request));
> + break;
> + default:
> + av_log(h, AV_LOG_ERROR, "Internal server error.\n");
"Unhandled HTTP error"? Your choice.
> + ffurl_write(s->hd, internal_server_error, strlen(internal_server_error));
> + }
> + }
> av_dict_free(&s->chained_options);
> return ret;
> }
> --
> 2.1.0
>
> From a1a13c83724f3b2ec0ee46efbf6e84e123e5538d Mon Sep 17 00:00:00 2001
> From: Stephan Holljes <klaxa1337 at googlemail.com>
> Date: Wed, 3 Jun 2015 20:41:50 +0200
> Subject: [PATCH 2/5] lavf/http: Process HTTP header before sending response.
>
> Signed-off-by: Stephan Holljes <klaxa1337 at googlemail.com>
> ---
> libavformat/http.c | 4 ++--
> 1 file changed, 2 insertions(+), 2 deletions(-)
LGTM. Will test when time permits.
> From 40cd84a08ab76fcf5d22e6ca47e3aa3cb4bc0bc1 Mon Sep 17 00:00:00 2001
> From: Stephan Holljes <klaxa1337 at googlemail.com>
> Date: Wed, 3 Jun 2015 20:26:37 +0200
> Subject: [PATCH 1/5] lavf/http: Document method option.
>
> Signed-off-by: Stephan Holljes <klaxa1337 at googlemail.com>
> ---
> doc/protocols.texi | 9 +++++++++
> libavformat/http.c | 2 +-
> 2 files changed, 10 insertions(+), 1 deletion(-)
>
> diff --git a/doc/protocols.texi b/doc/protocols.texi
> index f822d81..3a4dbb0 100644
> --- a/doc/protocols.texi
> +++ b/doc/protocols.texi
> @@ -278,6 +278,15 @@ Set initial byte offset.
> @item end_offset
> Try to limit the request to bytes preceding this offset.
>
> + at item method
> +When used as a client option it sets the HTTP method for the request.
> +
> +When used as a server option it sets the HTTP method that is going to be
> +expected from the client(s).
> +If the expected and the received HTTP method do not match the client will
> +be given a Bad Request response.
> +When unset the HTTP method is not checked.
Maybe add something like "not checked for now" or "but this may change", so
that people will not start relying on it. But this is very minor.
> +
> @item listen
> If set to 1 enables experimental HTTP server. This can be used to send data when
> used as an output option, or read data from a client with HTTP POST when used as
> diff --git a/libavformat/http.c b/libavformat/http.c
> index 4f6716a..546741a 100644
> --- a/libavformat/http.c
> +++ b/libavformat/http.c
> @@ -126,7 +126,7 @@ static const AVOption options[] = {
> { "location", "The actual location of the data received", OFFSET(location), AV_OPT_TYPE_STRING, { 0 }, 0, 0, D | E },
> { "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", OFFSET(method), AV_OPT_TYPE_STRING, { .str = NULL }, 0, 0, E },
> + { "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 },
> { "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, 1, D | E },
> { NULL }
LGTM apart from that.
Regards,
--
Nicolas George
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 819 bytes
Desc: Digital signature
URL: <https://ffmpeg.org/pipermail/ffmpeg-devel/attachments/20150603/a8e2c0b1/attachment.asc>
More information about the ffmpeg-devel
mailing list