[FFmpeg-devel] [PATCH][GSoC] lavf/http: Parse and set HTTP method when listening on HTTP(S)
Stephan Holljes
klaxa1337 at googlemail.com
Wed Jun 3 21:10:24 CEST 2015
On Tue, Jun 2, 2015 at 10:56 AM, Nicolas George <george at nsup.org> wrote:
> 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.
I haven't looked at this in-depth. A quick test didn't work as
expected, but I'll keep working on it.
>
>> 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.
OK, fixed.
>
>> 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.
500 error added.
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.
>
> 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.
>
>> 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?
Fixed.
>
>> +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.
Agreed, if unset the method is ignored for now. In the future
auto-detection should be used.
>
>> +
>> @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.
Fixed.
Also, the complete first line is now parsed for method, resource and
HTTP version.
>
> 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.
I was under the misconception that 0 meant "no errors occurred". Fixed.
>
>> + }
>> while (!av_isspace(*p) && *p != '\0')
>> p++;
>> while (av_isspace(*p))
>> --
>> 2.1.0
>>
>
> Regards,
>
> --
> Nicolas George
> _______________________________________________
> ffmpeg-devel mailing list
> ffmpeg-devel at ffmpeg.org
> http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
-------------- next part --------------
A non-text attachment was scrubbed...
Name: 0005-lavf-http-Indent-else-clause.patch
Type: text/x-patch
Size: 1437 bytes
Desc: not available
URL: <https://ffmpeg.org/pipermail/ffmpeg-devel/attachments/20150603/05e851cd/attachment.bin>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: 0004-lavf-http-Properly-process-HTTP-header-on-listen.patch
Type: text/x-patch
Size: 2802 bytes
Desc: not available
URL: <https://ffmpeg.org/pipermail/ffmpeg-devel/attachments/20150603/05e851cd/attachment-0001.bin>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: 0003-lavf-http-Rudimentary-error-handling-for-HTTP-reques.patch
Type: text/x-patch
Size: 1746 bytes
Desc: not available
URL: <https://ffmpeg.org/pipermail/ffmpeg-devel/attachments/20150603/05e851cd/attachment-0002.bin>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: 0002-lavf-http-Process-HTTP-header-before-sending-respons.patch
Type: text/x-patch
Size: 1034 bytes
Desc: not available
URL: <https://ffmpeg.org/pipermail/ffmpeg-devel/attachments/20150603/05e851cd/attachment-0003.bin>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: 0001-lavf-http-Document-method-option.patch
Type: text/x-patch
Size: 2251 bytes
Desc: not available
URL: <https://ffmpeg.org/pipermail/ffmpeg-devel/attachments/20150603/05e851cd/attachment-0004.bin>
More information about the ffmpeg-devel
mailing list