Re: [FFmpeg-devel] [PATCH]Add one CRLF to http headers if necessary
On Mon, Mar 16, 2015 at 04:04:16PM +0100, Nicolas George wrote: > Le primidi 21 ventôse, an CCXXIII, Michael Niedermayer a écrit : [...] > "\r\n", five exclamation marks or an ASCII-art tortoise. > > For the GUI application that you suggested earlier, and similar > applications: First, I consider them rather unlikely: exporting an expert > option in a GUI with a simple text entry is way too fragile. Still, let us > admit it exists. It must do a special case for HTTP headers, since it is a > multi-line field. In that case, doing a special case for sanitizing the > string is not too much to ask. Of course, lavu can help: I would have no > objection to some kind of "av_string_normalize_line_breaks(&str)" to do the > actual work. > > For command-line applications accepting the options directly from the user: > For Unix shells: benefit: the user can hit return without hitting Ctrl-V > Ctrl-M before, two extra keys. Not much. > For the windows shell: benefit: 0. I suspect hitting return with unclosed > quotes will not have the desired effect. And CRLF is the native windows line > ending anyway. > > The global benefit, as I see it, is rather feeble. > > I can propose a middle ground: > > Normalize line endings, but still print a warning. > > That way, lazy users can spare the few keys, but we are free to break it if > we need/want. ok, that sounds very acceptable to me thanks [...] -- Michael GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB The worst form of inequality is to try to make unequal things equal. -- Aristotle signature.asc Description: Digital signature ___ ffmpeg-devel mailing list ffmpeg-devel@ffmpeg.org http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
Re: [FFmpeg-devel] [PATCH]Add one CRLF to http headers if necessary
Le primidi 21 ventôse, an CCXXIII, Michael Niedermayer a écrit : > If the user passes just one header "line" then either he adds > CRLF at the end or he doesnt. These 2 cases can be easily distnguished > and teh code can add CRLF if its missing > this is basically carls patch and i think its good and simplifies > useage, For the case of a single line, I do not disagree. OTOH, I would prefer to see it implemented that way: if (need_extra_crlf) len += av_strlcpy(headers + len, "\r\n", sizeof(headers) - len); In other words: add the missing CRLF on the fly when using the s->headers option. > The case of multiple header lines is trickier, http uses CRLF as Let us focus on that one please. > seperator IIRC. My idea was to be a bit more flexible here and allow > any line ending character and normalize these to CRLF before sending > them. This is your first patch i belive. True, that was my first patch. > This would allow users to just use a quote and the enter key after each > header i assume on the command line. While in a textfield of a GUI > it can be entered easily 1 line per header. > Maybe iam missing something here but it seems convenient to allow > this than to strictly require CRLF In short: this is an API and an API must be strict. This is not just a hollow principle that I want to apply blindly. Once we document this is accepted, people will start to use it, and we will be stuck with it, we must maintain it and keep compatibility until at least two major bumps. This is the same reason for writing ">= for success" even if the function returns only 0: MAYBE sometime later we will want to expand, and in that case it will be an advantage to us. Of course, it has to be balanced with the benefits for the users. First, I would like to emphasize that the HTTP headers option would have the AV_OPT_EXPERT flag if it existed in lavu and not only in cmdutils. Now, let us see the various uses of the option and the corresponding benefits. For an application that builds the HTTP headers string from various sources: benefit: 0. A delimiter string is a delimiter string, whether it is "\n", "\r\n", five exclamation marks or an ASCII-art tortoise. For the GUI application that you suggested earlier, and similar applications: First, I consider them rather unlikely: exporting an expert option in a GUI with a simple text entry is way too fragile. Still, let us admit it exists. It must do a special case for HTTP headers, since it is a multi-line field. In that case, doing a special case for sanitizing the string is not too much to ask. Of course, lavu can help: I would have no objection to some kind of "av_string_normalize_line_breaks(&str)" to do the actual work. For command-line applications accepting the options directly from the user: For Unix shells: benefit: the user can hit return without hitting Ctrl-V Ctrl-M before, two extra keys. Not much. For the windows shell: benefit: 0. I suspect hitting return with unclosed quotes will not have the desired effect. And CRLF is the native windows line ending anyway. The global benefit, as I see it, is rather feeble. I can propose a middle ground: Normalize line endings, but still print a warning. That way, lazy users can spare the few keys, but we are free to break it if we need/want. Regards, -- Nicolas George signature.asc Description: Digital signature ___ ffmpeg-devel mailing list ffmpeg-devel@ffmpeg.org http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
Re: [FFmpeg-devel] [PATCH]Add one CRLF to http headers if necessary
Le decadi 20 ventôse, an CCXXIII, wm4 a écrit : > Why not fix the shortcomings in FFmpeg? (The option API.) https://lists.libav.org/pipermail/libav-devel/2015-March/067929.html Regards, -- Nicolas George signature.asc Description: Digital signature ___ ffmpeg-devel mailing list ffmpeg-devel@ffmpeg.org http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
Re: [FFmpeg-devel] [PATCH]Add one CRLF to http headers if necessary
On Wed, Mar 11, 2015 at 02:59:04PM +0100, Nicolas George wrote: > Le primidi 21 ventôse, an CCXXIII, Michael Niedermayer a écrit : > > the question or well rather my question is why do they need to put > > control characters in there in the first place. > > There should be no need for that > > Control characters are the only characters that are not valid in a HTTP > header value, therefore they are the only usable delimiters to group several > headers in a single string, unless you want to implement one more level of > escaping. > > Or do you have another solution in mind? well i sure do not want more escaping and yes i do, or lets say i think i do. If the user passes just one header "line" then either he adds CRLF at the end or he doesnt. These 2 cases can be easily distnguished and teh code can add CRLF if its missing this is basically carls patch and i think its good and simplifies useage, doing this at cmdutils level is not a good idea IMHO as other applications as well need to pass header strings from the user to lavf. and requiring every app, GUI based ones included to sanitize the string before passing it on or requireing the user to litterally add the CRLF somehow seems very inconvenient to me The case of multiple header lines is trickier, http uses CRLF as seperator IIRC. My idea was to be a bit more flexible here and allow any line ending character and normalize these to CRLF before sending them. This is your first patch i belive. This would allow users to just use a quote and the enter key after each header i assume on the command line. While in a textfield of a GUI it can be entered easily 1 line per header. Maybe iam missing something here but it seems convenient to allow this than to strictly require CRLF [...] -- Michael GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB Everything should be made as simple as possible, but not simpler. -- Albert Einstein signature.asc Description: Digital signature ___ ffmpeg-devel mailing list ffmpeg-devel@ffmpeg.org http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
Re: [FFmpeg-devel] [PATCH]Add one CRLF to http headers if necessary
Le primidi 21 ventôse, an CCXXIII, Michael Niedermayer a écrit : > the question or well rather my question is why do they need to put > control characters in there in the first place. > There should be no need for that Control characters are the only characters that are not valid in a HTTP header value, therefore they are the only usable delimiters to group several headers in a single string, unless you want to implement one more level of escaping. Or do you have another solution in mind? Regards, -- Nicolas George signature.asc Description: Digital signature ___ ffmpeg-devel mailing list ffmpeg-devel@ffmpeg.org http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
Re: [FFmpeg-devel] [PATCH]Add one CRLF to http headers if necessary
On Wed, Mar 11, 2015 at 01:24:28PM +0100, Nicolas George wrote: > Le primidi 21 ventôse, an CCXXIII, Michael Niedermayer a écrit : > > what is the problem ? > > can you elaborate? > > The problem raised in this trac ticket is that windows users seem unable to > put control characters in their command lines. the question or well rather my question is why do they need to put control characters in there in the first place. There should be no need for that [...] -- Michael GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB No human being will ever know the Truth, for even if they happen to say it by chance, they would not even known they had done so. -- Xenophanes signature.asc Description: Digital signature ___ ffmpeg-devel mailing list ffmpeg-devel@ffmpeg.org http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
Re: [FFmpeg-devel] [PATCH]Add one CRLF to http headers if necessary
Le primidi 21 ventôse, an CCXXIII, Michael Niedermayer a écrit : > what is the problem ? > can you elaborate? The problem raised in this trac ticket is that windows users seem unable to put control characters in their command lines. I thought it was a shortcoming of the windows command line interpreter, but I have come to suspect it may be a deeper flaw in the way command-line arguments are passed to programs. I have submitted the question to a friend who knows windows well. > theres a bug in OUR code it should be fixed in our code. No, there is no bug in our code. The issue is definitely not a bug in lavf: lavf takes a C string, and except for \0, a C string can contain any character without any problem. And this API is not even as bad as some people claim: passing a list of values is always clumsy. It may be a shortcoming in our command-line interface IF AND ONLY IF it is established that some OSes can not pass arbitrary C string as arguments to main(). A shortcoming, not a bug. > The line ending characters differ between platforms and users do in > general not know about these differences nor should they need to. You seem to be under the misapprehension the issue is about different line ending encodings. It is not, it never was. > IMO > User interfaces should be designed so they do what the user wants with > minimun effort on the users part. True, but this "minimum effort" must be measured globally, not locally. It may seem convenient to make a special case for this or that option. If you only look at the effort required to invoke that particular option, that is true. But you also have to count the effort to remember which option has a special case. When you have hundreds of options like FFmpeg, you want, as much as possible, that ALL OPTIONS BEHAVE EXACTLY THE SAME. Any other consideration is secondary. Regards, -- Nicolas George signature.asc Description: Digital signature ___ ffmpeg-devel mailing list ffmpeg-devel@ffmpeg.org http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
Re: [FFmpeg-devel] [PATCH]Add one CRLF to http headers if necessary
On Tue, Mar 10, 2015 at 02:09:10PM +0100, Nicolas George wrote: > Le decadi 20 ventôse, an CCXXIII, Carl Eugen Hoyos a écrit : > > Please add a reference to ticket #3268 > > to the commit message. > > I was about to reply "locally added", but no, because it does not fix that > ticket. what is the problem ? can you elaborate? > > Re-thinking on the whole discussion, I withdraw this patch, it is wrong. This > part of the code is an API, not an UI, so it is better if it is strict. I > will try to propose another patch for pure validation. > > Regarding the trac ticket itself, I wrote this, and I stick to it: > > # I am not in favour of fixing the shortcomings of the user's shell in > # FFmpeg's libraries > > # If we are talking about end-user interface, the changes should happen in > # cmdutils.c. > > In other words, I vote for closing #3268 as WONTFIX and suggesting windows > users to use a better shell (maybe the so-called powershell introduced in > the recent windows versions can do it). theres a bug in OUR code it should be fixed in our code. The line ending characters differ between platforms and users do in general not know about these differences nor should they need to. the command line input will in the overwhelming number of cases be in the platforms native "line ending encoding". the strings passed on API level will certainly often be in the platforms native encoding as well, in the case of headers they might be in the CRLF encoding too, and considering that these are generic options, some applications will export them generically and not have special cases to re encode the strings, and i think quite independant of the option passing API. > > If people really want to fix microsoft's shortcomings in FFmpeg, then it > must happen in the UI, not the API, i.e. in cmdutils.c. Maybe something like > that: "ffmpeg ... -expand -headers 'Cookies: chocolate\r\n'" (-expand being > a new option meaning "perform escape-character expansion on the following > option argument"). But I do not intend to work on it, because anybody can > get a shell where $'\r\n' works. I think thats alot of effort on the users part you ask here for IMO User interfaces should be designed so they do what the user wants with minimun effort on the users part. Aka i belive its not the human who should have to adapt to interface to a computer (in the extreemest case that would be flipping bits one by one in memory) but rather the computer should adapt to how humans interact where it is possible and works reliable [...] -- Michael GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB The real ebay dictionary, page 1 "Used only once"- "Some unspecified defect prevented a second use" "In good condition" - "Can be repaird by experienced expert" "As is" - "You wouldnt want it even if you were payed for it, if you knew ..." signature.asc Description: Digital signature ___ ffmpeg-devel mailing list ffmpeg-devel@ffmpeg.org http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
Re: [FFmpeg-devel] [PATCH]Add one CRLF to http headers if necessary
Le decadi 20 ventôse, an CCXXIII, Nicolas George a écrit : > I will try to propose another patch for pure validation. See the attached patches. Regards, -- Nicolas George From cef95aaec223b6644db6e52f9c6452d7aa01648b Mon Sep 17 00:00:00 2001 From: Nicolas George Date: Tue, 10 Mar 2015 13:43:17 +0100 Subject: [PATCH 1/2] lavf/http: full validation of headers option string. Signed-off-by: Nicolas George --- libavformat/http.c | 58 -- 1 file changed, 52 insertions(+), 6 deletions(-) diff --git a/libavformat/http.c b/libavformat/http.c index 55dcb6e..2d8f5dc 100644 --- a/libavformat/http.c +++ b/libavformat/http.c @@ -25,6 +25,7 @@ #include #endif /* CONFIG_ZLIB */ +#include "libavutil/avassert.h" #include "libavutil/avstring.h" #include "libavutil/opt.h" @@ -292,6 +293,55 @@ int ff_http_averror(int status_code, int default_averror) return default_averror; } +static void validate_headers_option(void *log, unsigned char *header) +{ +unsigned char *p = header; +const char *error = NULL; +int tag; + +/* *( header-field CRLF ) */ +while (*p) { +/* header-field = field-name ":" OWS field-value OWS + field-name = token = 1*tchar */ +tag = 0; +while ((unsigned)((*p | 32) - 'a') < 26 || (unsigned)(*p - '0') < 10 || + *p == '!' || *p == '#' || *p == '$' || *p == '%' || *p == '&' || + *p =='\'' || *p == '*' || *p == '+' || *p == '-' || *p == '.' || + *p == '^' || *p == '_' || *p == '`' || *p == '|' || *p == '~') { +p++; +tag = 1; +} +if (!tag || *p != ':') { +error = "no valid field name"; +goto fail; +} +/* field-value = *( field-content / obs-fold ) + field-content = field-vchar [ 1*( SP / HTAB ) field-vchar ] + field-vchar = VCHAR / obs-text + VCHAR = visible USASCII character + obs-text = %x80-FF + obs-fold = CRLF 1*( SP / HTAB ) */ +while (*p) { +while (*p >= 32 || *p == '\t') +p++; +if (p[0] != '\r' || p[1] != '\n') { +error = *p ? "invalid character in vield value" : + "missing trailing CRLF"; +goto fail; +} +p += 2; +if (*p != ' ' && *p != '\t') +break; +p++; +} +} +return; + +fail: +av_log(log, AV_LOG_WARNING, "Invalid HTTP header at offset %u: %s\n", + (unsigned)(p - header), error); +} + static int http_open(URLContext *h, const char *uri, int flags, AVDictionary **options) { @@ -310,12 +360,8 @@ static int http_open(URLContext *h, const char *uri, int flags, if (options) av_dict_copy(&s->chained_options, *options, 0); -if (s->headers) { -int len = strlen(s->headers); -if (len < 2 || strcmp("\r\n", s->headers + len - 2)) -av_log(h, AV_LOG_WARNING, - "No trailing CRLF found in HTTP header.\n"); -} +if (s->headers) +validate_headers_option(s, s->headers); ret = http_open_cnx(h, options); if (ret < 0) -- 2.1.4 From 68591d1dc097cae158eb8bcb08abc884dc414a3d Mon Sep 17 00:00:00 2001 From: Nicolas George Date: Wed, 11 Mar 2015 11:50:21 +0100 Subject: [PATCH 2/2] doc/protocols: insist on HTTP headers syntax. Signed-off-by: Nicolas George --- doc/protocols.texi | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/doc/protocols.texi b/doc/protocols.texi index 5f6dfa8..b46a36d 100644 --- a/doc/protocols.texi +++ b/doc/protocols.texi @@ -230,7 +230,8 @@ Set a specific content type for the POST messages. @item headers Set custom HTTP headers, can override built in default headers. The -value must be a string encoding the headers. +value must be a string encoding the headers, with lines terminated by CRLF, +including the final one. @item multiple_requests Use persistent connections if set to 1, default is 0. -- 2.1.4 signature.asc Description: Digital signature ___ ffmpeg-devel mailing list ffmpeg-devel@ffmpeg.org http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
Re: [FFmpeg-devel] [PATCH]Add one CRLF to http headers if necessary
On Tue, 10 Mar 2015 14:09:10 +0100 Nicolas George wrote: > Le decadi 20 ventôse, an CCXXIII, Carl Eugen Hoyos a écrit : > > Please add a reference to ticket #3268 > > to the commit message. > > I was about to reply "locally added", but no, because it does not fix that > ticket. > > Re-thinking on the whole discussion, I withdraw this patch, it is wrong. This > part of the code is an API, not an UI, so it is better if it is strict. I > will try to propose another patch for pure validation. > > Regarding the trac ticket itself, I wrote this, and I stick to it: > > # I am not in favour of fixing the shortcomings of the user's shell in > # FFmpeg's libraries > > # If we are talking about end-user interface, the changes should happen in > # cmdutils.c. > > In other words, I vote for closing #3268 as WONTFIX and suggesting windows > users to use a better shell (maybe the so-called powershell introduced in > the recent windows versions can do it). > > If people really want to fix microsoft's shortcomings in FFmpeg, then it > must happen in the UI, not the API, i.e. in cmdutils.c. Maybe something like > that: "ffmpeg ... -expand -headers 'Cookies: chocolate\r\n'" (-expand being > a new option meaning "perform escape-character expansion on the following > option argument"). But I do not intend to work on it, because anybody can > get a shell where $'\r\n' works. > > Regards, > Why not fix the shortcomings in FFmpeg? (The option API.) ___ ffmpeg-devel mailing list ffmpeg-devel@ffmpeg.org http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
Re: [FFmpeg-devel] [PATCH]Add one CRLF to http headers if necessary
Le decadi 20 ventôse, an CCXXIII, Carl Eugen Hoyos a écrit : > Please add a reference to ticket #3268 > to the commit message. I was about to reply "locally added", but no, because it does not fix that ticket. Re-thinking on the whole discussion, I withdraw this patch, it is wrong. This part of the code is an API, not an UI, so it is better if it is strict. I will try to propose another patch for pure validation. Regarding the trac ticket itself, I wrote this, and I stick to it: # I am not in favour of fixing the shortcomings of the user's shell in # FFmpeg's libraries # If we are talking about end-user interface, the changes should happen in # cmdutils.c. In other words, I vote for closing #3268 as WONTFIX and suggesting windows users to use a better shell (maybe the so-called powershell introduced in the recent windows versions can do it). If people really want to fix microsoft's shortcomings in FFmpeg, then it must happen in the UI, not the API, i.e. in cmdutils.c. Maybe something like that: "ffmpeg ... -expand -headers 'Cookies: chocolate\r\n'" (-expand being a new option meaning "perform escape-character expansion on the following option argument"). But I do not intend to work on it, because anybody can get a shell where $'\r\n' works. Regards, -- Nicolas George signature.asc Description: Digital signature ___ ffmpeg-devel mailing list ffmpeg-devel@ffmpeg.org http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
Re: [FFmpeg-devel] [PATCH]Add one CRLF to http headers if necessary
Nicolas George nsup.org> writes: > What about the attached patch? Please add a reference to ticket #3268 to the commit message. Thank you, Carl Eugen ___ ffmpeg-devel mailing list ffmpeg-devel@ffmpeg.org http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
Re: [FFmpeg-devel] [PATCH]Add one CRLF to http headers if necessary
Le decadi 20 ventôse, an CCXXIII, Michael Niedermayer a écrit : > iam fine with either, i was just trying to be completely correct > which may be overkill and indeed make the code less readable for no > gain What about the attached patch? Regards, -- Nicolas George From 28fc644a1d7e4f73921bbc9ab7c103de922480f1 Mon Sep 17 00:00:00 2001 From: Nicolas George Date: Tue, 10 Mar 2015 13:43:17 +0100 Subject: [PATCH] lavf/http: cleanup headers option string. Replace any sequence of CR or LF with a single CRLF and make sure there is a final CRLF. Signed-off-by: Nicolas George --- libavformat/http.c | 65 ++ 1 file changed, 65 insertions(+) diff --git a/libavformat/http.c b/libavformat/http.c index 55dcb6e..5371e92 100644 --- a/libavformat/http.c +++ b/libavformat/http.c @@ -25,6 +25,7 @@ #include #endif /* CONFIG_ZLIB */ +#include "libavutil/avassert.h" #include "libavutil/avstring.h" #include "libavutil/opt.h" @@ -292,6 +293,67 @@ int ff_http_averror(int status_code, int default_averror) return default_averror; } +/** + * Cleanup HTTP headers option string + * Replace any sequence of CR or LF with a single CRLF (empty lines are not + * valid in HTTP) and make sure there is a final CRLF. + */ +static int cleanup_headers_option(char **str) +{ +char *in = *str, *out, *p, *q; +size_t nb_chars = 0, nb_lf = 0, out_size; +int clean = 1, eol = 0; + +if (!in) +return 0; +p = in; +while (*p) { +if (*p == '\n' || *p == '\r') { +nb_lf++; +eol = 1; +if (p[0] != '\r' || p[1] != '\n' || p[2] == '\n' || p[2] == '\r') +clean = 0; +while (*p == '\n' || *p == '\r') +p++; +} else { +nb_chars++; +eol = 0; +p++; +} +} +if (clean && eol) +return 0; +if (!eol) +nb_lf++; +if (nb_lf >= (SIZE_MAX - nb_chars) / 2 - 1) +return AVERROR(ENOMEM); +out_size = nb_chars + nb_lf * 2 + 1; +out = av_realloc(NULL, out_size); +if (!out) +return AVERROR(ENOMEM); +p = in; +q = out; +while (*p) { +if (*p == '\n' || *p == '\r') { +*(q++) = '\r'; +*(q++) = '\n'; +while (*p == '\n' || *p == '\r') +p++; +} else { +*(q++) = *(p++); +} +} +if (!eol) { +*(q++) = '\r'; +*(q++) = '\n'; +} +*(q++) = 0; +av_assert0(q == out + out_size); +av_free(in); +*str = out; +return 0; +} + static int http_open(URLContext *h, const char *uri, int flags, AVDictionary **options) { @@ -310,6 +372,9 @@ static int http_open(URLContext *h, const char *uri, int flags, if (options) av_dict_copy(&s->chained_options, *options, 0); +ret = cleanup_headers_option(&s->headers); +if (ret < 0) +return ret; if (s->headers) { int len = strlen(s->headers); if (len < 2 || strcmp("\r\n", s->headers + len - 2)) -- 2.1.4 signature.asc Description: Digital signature ___ ffmpeg-devel mailing list ffmpeg-devel@ffmpeg.org http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
Re: [FFmpeg-devel] [PATCH]Add one CRLF to http headers if necessary
On Tue, Mar 10, 2015 at 09:39:50AM +0100, Nicolas George wrote: > Le nonidi 19 ventôse, an CCXXIII, Michael Niedermayer a écrit : > > there should be a loop that replaces each sequence of any \n\r by a single > > crlf sequency > > > also ensure that one such sequence is at the end > > I believe this would be correct. > > > using no \r \n codes but only litteral bytes and > > This part would also be correct, but I believe it is completely unnecessary: > POSIX specifies the byte value of \r and \n (the C standard does not), and > AFAIK we never had bug reports that could even remotely be linked to that > issue. And seriously, if a compiler were to use different values, FATE would > fail all over the place immediately. > > It does not matter much, since this is equivalent, but I believe most C > programmers will find \n and \r much more readable than \012 and \015 or > even \x0a and \x0d; I do. iam fine with either, i was just trying to be completely correct which may be overkill and indeed make the code less readable for no gain [...] -- Michael GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB It is dangerous to be right in matters on which the established authorities are wrong. -- Voltaire signature.asc Description: Digital signature ___ ffmpeg-devel mailing list ffmpeg-devel@ffmpeg.org http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
Re: [FFmpeg-devel] [PATCH]Add one CRLF to http headers if necessary
Le nonidi 19 ventôse, an CCXXIII, Michael Niedermayer a écrit : > there should be a loop that replaces each sequence of any \n\r by a single > crlf sequency > also ensure that one such sequence is at the end I believe this would be correct. > using no \r \n codes but only litteral bytes and This part would also be correct, but I believe it is completely unnecessary: POSIX specifies the byte value of \r and \n (the C standard does not), and AFAIK we never had bug reports that could even remotely be linked to that issue. And seriously, if a compiler were to use different values, FATE would fail all over the place immediately. It does not matter much, since this is equivalent, but I believe most C programmers will find \n and \r much more readable than \012 and \015 or even \x0a and \x0d; I do. Regards, -- Nicolas George signature.asc Description: Digital signature ___ ffmpeg-devel mailing list ffmpeg-devel@ffmpeg.org http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
Re: [FFmpeg-devel] [PATCH]Add one CRLF to http headers if necessary
Michael Niedermayer gmx.at> writes: > if you only want t fix the end, then existing \r > and \n should be stripped and crlf be added > litterally without use of \r\n but literal byte > values On which system can I test this (so that it actually makes a difference)? Thank you, Carl Eugen ___ ffmpeg-devel mailing list ffmpeg-devel@ffmpeg.org http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
Re: [FFmpeg-devel] [PATCH]Add one CRLF to http headers if necessary
On Thu, Feb 26, 2015 at 04:28:54PM +0100, Carl Eugen Hoyos wrote: > On Thursday 26 February 2015 03:31:39 pm Derek Buitenhuis wrote: > > On 2/26/2015 2:09 PM, Carl Eugen Hoyos wrote: > > > +snprintf(header, len + 3, "%s\r\n", s->headers); > > > > This does not necessarily work on windows. The C standard idctates that in > > text mode, \n is translated to the system's native newline. > > > > Use memcpy and 0x0D / 0X0A / 0x00. > > New patch attached. > > > This may also accidentally allow for headers to end with '\n\r\n', > > wouldn't it? > > Yes, I don't know if this is a problem. > > Thank you, Carl Eugen > http.c | 12 ++-- > 1 file changed, 10 insertions(+), 2 deletions(-) > 6114a18a8dd0b1737c844b18a839a6ef99617145 patchhttpcrlf2.diff > diff --git a/libavformat/http.c b/libavformat/http.c > index 55dcb6e..59e5acb 100644 > --- a/libavformat/http.c > +++ b/libavformat/http.c > @@ -312,9 +312,17 @@ static int http_open(URLContext *h, const char *uri, int > flags, > > if (s->headers) { > int len = strlen(s->headers); > -if (len < 2 || strcmp("\r\n", s->headers + len - 2)) > +if (len < 2 || strcmp("\r\n", s->headers + len - 2)) { > +char *header = av_malloc(len + 3); > +if (!header) > +return AVERROR(ENOMEM); > +memcpy(header, s->headers, len); > +memcpy(header + len, "\r\n\0", 3); there should be a loop that replaces each sequence of any \n\r by a single crlf sequency using no \r \n codes but only litteral bytes and also ensure that one such sequence is at the end if you only want t fix the end, then existing \r and \n should be stripped and crlf be added litterally without use of \r\n but literal byte values to repeat, the input checks should use \r\n the output should not we assume the user would provide headers in the platforms encoding while we need http encoding on the output side i hope above is correct and not missing something [...] -- Michael GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB It is what and why we do it that matters, not just one of them. signature.asc Description: Digital signature ___ ffmpeg-devel mailing list ffmpeg-devel@ffmpeg.org http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
Re: [FFmpeg-devel] [PATCH]Add one CRLF to http headers if necessary
Carl Eugen Hoyos ag.or.at> writes: > I believe attached patch fixes ticket #3268 in some cases. Ping, should one of the patches get applied? Carl Eugen ___ ffmpeg-devel mailing list ffmpeg-devel@ffmpeg.org http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
Re: [FFmpeg-devel] [PATCH]Add one CRLF to http headers if necessary
On 2/26/2015 4:04 PM, Nicolas George wrote: > If we are talking about end-user interface, the changes should happen in > cmdutils.c. Fair enough. - Derek ___ ffmpeg-devel mailing list ffmpeg-devel@ffmpeg.org http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
Re: [FFmpeg-devel] [PATCH]Add one CRLF to http headers if necessary
L'octidi 8 ventôse, an CCXXIII, Derek Buitenhuis a écrit : > I read it as a 'text stream', which isn't necessarily file i/o only, but I > doubt it is worth bikeshedding over. You are probably right, I meant "file" I/O in the Unix sense. > I think relying on the user to input a CR+LF cia command line program is > insane in the first place... shouldn't the library do that / join headers > properly? The major issue there is that the type of the "headers" option is really "list of pairs of strings". The AVOption system does not have anything like that, not even "list of strings", which would be the second best choice. Lacking that, we have to use the next best type, "string", and require the user to serialize the "list of pairs of strings" into a single string. And since a line break is the only character that can not be present in the header value, using it as delimiter is a sane choice. Remember that we are talking about library API, not end-user interface. CR+LF is not harder to insert than any other sequence of bytes. If we are talking about end-user interface, the changes should happen in cmdutils.c. Regards, -- Nicolas George signature.asc Description: Digital signature ___ ffmpeg-devel mailing list ffmpeg-devel@ffmpeg.org http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
Re: [FFmpeg-devel] [PATCH]Add one CRLF to http headers if necessary
On 2/26/2015 3:35 PM, Nicolas George wrote: > The second issue is that with the windows file API, when the file is opened > in the so-called "text mode", there is an implicit conversion between '\n' > in memory (which is actually exactly 0x0A) and "\r\n" in the file; there is > also a conversion between 0x1A and EOF. It only happens with file I/O > operations, not with simple string operations, even sprintf. On Unix, a > similar conversion happens in the TTY layer. The TTY layer is something > nobody should touch without a long pole and a hazmat suit; on windows, the > hazmat is littered all over the API. I read it as a 'text stream', which isn't necessarily file i/o only, but I doubt it is worth bikeshedding over. > Therefore, I believe Carl Eugen's original patch was technically correct. See below. >> This may also accidentally allow for headers to end with '\n\r\n', >> wouldn't it? > > Well, the test as it is is half-assed, it checks for a specific common > mistake but does not actually validate the format of the string. Yes it's pretty bad, more on that below. > I am not in favour of fixing the shortcomings of the user's shell in > FFmpeg's libraries, but if it is considered useful, I believe a more > complete approach should be used: replace all occurrences of \n with CRLF, > validate folded headers, etc. I think relying on the user to input a CR+LF cia command line program is insane in the first place... shouldn't the library do that / join headers properly? - Derek ___ ffmpeg-devel mailing list ffmpeg-devel@ffmpeg.org http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
Re: [FFmpeg-devel] [PATCH]Add one CRLF to http headers if necessary
On Thursday 26 February 2015 03:31:39 pm Derek Buitenhuis wrote: > On 2/26/2015 2:09 PM, Carl Eugen Hoyos wrote: > > +snprintf(header, len + 3, "%s\r\n", s->headers); > > This does not necessarily work on windows. The C standard idctates that in > text mode, \n is translated to the system's native newline. > > Use memcpy and 0x0D / 0X0A / 0x00. New patch attached. > This may also accidentally allow for headers to end with '\n\r\n', > wouldn't it? Yes, I don't know if this is a problem. Thank you, Carl Eugen diff --git a/libavformat/http.c b/libavformat/http.c index 55dcb6e..59e5acb 100644 --- a/libavformat/http.c +++ b/libavformat/http.c @@ -312,9 +312,17 @@ static int http_open(URLContext *h, const char *uri, int flags, if (s->headers) { int len = strlen(s->headers); -if (len < 2 || strcmp("\r\n", s->headers + len - 2)) +if (len < 2 || strcmp("\r\n", s->headers + len - 2)) { +char *header = av_malloc(len + 3); +if (!header) +return AVERROR(ENOMEM); +memcpy(header, s->headers, len); +memcpy(header + len, "\r\n\0", 3); +av_free(s->headers); +s->headers = header; av_log(h, AV_LOG_WARNING, - "No trailing CRLF found in HTTP header.\n"); + "No trailing CRLF found in HTTP header, added \\r\\n at the end.\n"); +} } ret = http_open_cnx(h, options); ___ ffmpeg-devel mailing list ffmpeg-devel@ffmpeg.org http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
Re: [FFmpeg-devel] [PATCH]Add one CRLF to http headers if necessary
L'octidi 8 ventôse, an CCXXIII, Derek Buitenhuis a écrit : > This does not necessarily work on windows. The C standard idctates that in > text mode, \n is translated to the system's native newline. > > Use memcpy and 0x0D / 0X0A / 0x00. I think you are confusing two subtly different issues and getting the result wrong. The first issue is that the C standard does not specify the byte value of '\n' and '\r', the most notable example being macos<10 where '\n' is 0x0D instead of 0x0D everywhere else because apple never does anything like everyone else. But they still translate into a single byte value each, and POSIX specifies that '\n' = 0x0A and '\r' = 0x0D. The second issue is that with the windows file API, when the file is opened in the so-called "text mode", there is an implicit conversion between '\n' in memory (which is actually exactly 0x0A) and "\r\n" in the file; there is also a conversion between 0x1A and EOF. It only happens with file I/O operations, not with simple string operations, even sprintf. On Unix, a similar conversion happens in the TTY layer. The TTY layer is something nobody should touch without a long pole and a hazmat suit; on windows, the hazmat is littered all over the API. Therefore, I believe Carl Eugen's original patch was technically correct. > This may also accidentally allow for headers to end with '\n\r\n', > wouldn't it? Well, the test as it is is half-assed, it checks for a specific common mistake but does not actually validate the format of the string. I am not in favour of fixing the shortcomings of the user's shell in FFmpeg's libraries, but if it is considered useful, I believe a more complete approach should be used: replace all occurrences of \n with CRLF, validate folded headers, etc. Regards, -- Nicolas George signature.asc Description: Digital signature ___ ffmpeg-devel mailing list ffmpeg-devel@ffmpeg.org http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
Re: [FFmpeg-devel] [PATCH]Add one CRLF to http headers if necessary
On 2/26/2015 2:09 PM, Carl Eugen Hoyos wrote: > +snprintf(header, len + 3, "%s\r\n", s->headers); This does not necessarily work on windows. The C standard idctates that in text mode, \n is translated to the system's native newline. Use memcpy and 0x0D / 0X0A / 0x00. This may also accidentally allow for headers to end with '\n\r\n', wouldn't it? - Derek ___ ffmpeg-devel mailing list ffmpeg-devel@ffmpeg.org http://ffmpeg.org/mailman/listinfo/ffmpeg-devel