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