On Tue, Jun 2, 2015 at 10:56 AM, Nicolas George <geo...@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 <klaxa1...@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 <klaxa1...@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 <klaxa1...@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 <klaxa1...@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.
>>
>> +@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@ffmpeg.org
> http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
From 3af5c5867672624d45de447d11c2107b3c77742c Mon Sep 17 00:00:00 2001
From: Stephan Holljes <klaxa1...@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 <klaxa1...@googlemail.com>
---
libavformat/http.c | 16 ++++++++--------
1 file changed, 8 insertions(+), 8 deletions(-)
diff --git a/libavformat/http.c b/libavformat/http.c
index dfe2616..d853a71 100644
--- a/libavformat/http.c
+++ b/libavformat/http.c
@@ -610,16 +610,16 @@ static int process_line(URLContext *h, char *line, int line_count,
}
av_log(h, AV_LOG_TRACE, "HTTP version string: %s\n", version);
} else {
- while (!av_isspace(*p) && *p != '\0')
- p++;
- while (av_isspace(*p))
- p++;
- s->http_code = strtol(p, &end, 10);
+ while (!av_isspace(*p) && *p != '\0')
+ p++;
+ while (av_isspace(*p))
+ p++;
+ s->http_code = strtol(p, &end, 10);
- av_log(h, AV_LOG_TRACE, "http_code=%d\n", s->http_code);
+ av_log(h, AV_LOG_TRACE, "http_code=%d\n", s->http_code);
- if ((ret = check_http_code(h, s->http_code, end)) < 0)
- return ret;
+ if ((ret = check_http_code(h, s->http_code, end)) < 0)
+ return ret;
}
} else {
while (*p != '\0' && *p != ':')
--
2.1.0
From 90f17ce9711304228bc3b7bf8cb4cdfac6b14011 Mon Sep 17 00:00:00 2001
From: Stephan Holljes <klaxa1...@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 <klaxa1...@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++;
+ method = p;
+ while (!av_isspace(*p))
+ p++;
+ *p = '\0';
+ 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++;
+
+ // 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 <klaxa1...@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 <klaxa1...@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");
+ 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 <klaxa1...@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 <klaxa1...@googlemail.com>
---
libavformat/http.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/libavformat/http.c b/libavformat/http.c
index 546741a..e51f524 100644
--- a/libavformat/http.c
+++ b/libavformat/http.c
@@ -318,10 +318,10 @@ 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:
--
2.1.0
From 40cd84a08ab76fcf5d22e6ca47e3aa3cb4bc0bc1 Mon Sep 17 00:00:00 2001
From: Stephan Holljes <klaxa1...@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 <klaxa1...@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.
+@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.
+
@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 }
--
2.1.0
_______________________________________________
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
