[FFmpeg-devel] [PATCH][GSoC] lavf/http: Parse and set HTTP method when listening on HTTP(S)

[Nicolas George]

Le quartidi 14 prairial, an CCXXIII, Stephan Holljes a écrit :
> I agree, I have probably thought a little bit too far ahead.
> 
> I hope the attached patches are okay.
> I'm somewhat unsure about the first patch, because it checks ret and
> sets it explicitly if no other error occurred.
> I also added documentation for the method option in HTTPContext.

See remarks below.

> A related issue is how to autodetect which method is to be expected
> when it is not explicitly set.
> For example, when listening for a client as an input, a POST request
> should be expected, but how do I know that inside http.c?

I suspect you must look at AVIO_FLAG_WRITE and AVIO_FLAG_READ: if WRITE is
set, then the user must use a request with a body.

> From 8188b8a3b0cf21df7b80e410fd58b26f974db29e Mon Sep 17 00:00:00 2001
> From: Stephan Holljes <klaxa1337 at googlemail.com>
> Date: Tue, 2 Jun 2015 01:35:26 +0200
> Subject: [PATCH 1/2] lavf/http: Process client HTTP request header before
>  sending response and handle bad requests.
> 
> Signed-off-by: Stephan Holljes <klaxa1337 at googlemail.com>
> ---
>  libavformat/http.c | 10 ++++++++--
>  1 file changed, 8 insertions(+), 2 deletions(-)
> 
> diff --git a/libavformat/http.c b/libavformat/http.c
> index 4f6716a..6c18243 100644
> --- a/libavformat/http.c
> +++ b/libavformat/http.c
> @@ -304,6 +304,7 @@ 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\n\r\n";

IIRC, a valid HTTP response should contain a minimal body, with a content
type. Even if it is not a hard requirement, it is probably a good practice.

>      char hostname[1024], proto[10];
>      char lower_url[100];
>      const char *lower_proto = "tcp";
> @@ -318,13 +319,18 @@ static int http_listen(URLContext *h, const char *uri, int flags,
>      if ((ret = ffurl_open(&s->hd, lower_url, AVIO_FLAG_READ_WRITE,
>                            &h->interrupt_callback, options)) < 0)
>          goto fail;
> -    if ((ret = ffurl_write(s->hd, header, strlen(header))) < 0)
> -        goto fail;
>      if ((ret = http_read_header(h, &new_location)) < 0)
>           goto fail;
> +     if ((ret = ffurl_write(s->hd, header, strlen(header))) < 0)
> +         goto fail;
>      return 0;
>  
>  fail:

> +    if (ret == AVERROR_HTTP_BAD_REQUEST) {
> +        if ((ret = ffurl_write(s->hd, bad_request, strlen(bad_request))) < 0)
> +            goto fail;
> +        ret = AVERROR_HTTP_BAD_REQUEST;
> +    }

An error when writing the bad_request response will lead to a second attempt
at writing, and it will probably make an error again.

IMHO, it is ok to ignore write errors when sending error responses.

Errors that are not handler by the code should probably return some kind of
"500 Internal server error" response.

And it would probably be a good idea to isolate that part in a separate
function.

>      av_dict_free(&s->chained_options);
>      return ret;
>  }
> -- 
> 2.1.0
> 

> From 3263509fa862c944acc95fd61ba524b1c31545ca Mon Sep 17 00:00:00 2001
> From: Stephan Holljes <klaxa1337 at googlemail.com>
> Date: Tue, 2 Jun 2015 01:37:47 +0200
> Subject: [PATCH 2/2] lavf/http: Process client HTTP methods and document
>  method option.
> 
> Signed-off-by: Stephan Holljes <klaxa1337 at googlemail.com>
> ---
>  doc/protocols.texi | 10 ++++++++++
>  libavformat/http.c | 16 +++++++++++++++-
>  2 files changed, 25 insertions(+), 1 deletion(-)
> 
> diff --git a/doc/protocols.texi b/doc/protocols.texi
> index f822d81..d5ae332 100644
> --- a/doc/protocols.texi
> +++ b/doc/protocols.texi
> @@ -278,6 +278,16 @@ 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 by the client(s).

Expected *from* the clients?

> +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 used by the first client will be used for
> +comparison to later clients.

Are you sure this is a good idea? When multi-client is implemented, you want
different clients to be able to use different methods: some GET, some POST.

> +
>  @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 6c18243..6ef9f0c 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 }
> @@ -562,6 +562,20 @@ static int process_line(URLContext *h, char *line, int line_count,
>  
>      p = line;
>      if (line_count == 0) {
> +        if (s->listen) {
> +            if (s->method) {

> +                if (av_strncasecmp(s->method, line, strlen(s->method))) {

If I read the code properly, if line is "GETLOST / HTTP/1.0" and s->method
is "GET", it will work.

It is probably better to find the end of the method with the av_isspace()
you used below, put a \0 to terminate it, and use a plain av_strcasecmp()
after that.

> +                    av_log(h, AV_LOG_ERROR, "Received and expected HTTP method do not match.\n");
> +                    return ff_http_averror(400, AVERROR(EIO));
> +                }
> +                return 0;
> +            }
> +            while (!av_isspace(*p))
> +                p++;
> +            if (!(s->method = av_strndup(line, p - line)))
> +                return AVERROR(ENOMEM);

> +            return 0;

I am not sure about the "return 0", it seems it will stop processing the
headers. Using a else clause (with a separate patch to reindent it) would
probably be more readable.

> +        }
>          while (!av_isspace(*p) && *p != '\0')
>              p++;
>          while (av_isspace(*p))
> -- 
> 2.1.0
> 

Regards,

-- 
  Nicolas George