Re: [FFmpeg-devel] [PATCH]Add one CRLF to http headers if necessary

2015-03-16 Thread Michael Niedermayer 
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.

put some joke about how a computer can recognize ASCII-art tortoises
here


 
 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

2015-03-16 Thread Nicolas George 
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

2015-03-12 Thread Nicolas George 
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

2015-03-11 Thread Nicolas George 
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 geo...@nsup.org
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 geo...@nsup.org
---
 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 zlib.h
 #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 geo...@nsup.org
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 geo...@nsup.org
---
 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

2015-03-11 Thread Michael Niedermayer 
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

2015-03-11 Thread Nicolas George 
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

2015-03-11 Thread Michael Niedermayer 
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

2015-03-11 Thread Nicolas George 
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

2015-03-11 Thread Michael Niedermayer 
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

2015-03-10 Thread Michael Niedermayer 
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

2015-03-10 Thread Nicolas George 
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

2015-03-10 Thread Carl Eugen Hoyos 
Nicolas George george at 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

2015-03-10 Thread Nicolas George 
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 geo...@nsup.org
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 geo...@nsup.org
---
 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 zlib.h
 #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

2015-03-10 Thread wm4 
On Tue, 10 Mar 2015 14:09:10 +0100
Nicolas George geo...@nsup.org 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

2015-03-10 Thread Carl Eugen Hoyos 
Michael Niedermayer michaelni at 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

2015-03-10 Thread Nicolas George 
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

2015-03-09 Thread Michael Niedermayer 
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

2015-03-09 Thread Carl Eugen Hoyos 
Carl Eugen Hoyos cehoyos at 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

2015-02-26 Thread Derek Buitenhuis 
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

2015-02-26 Thread Nicolas George 
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

2015-02-26 Thread Carl Eugen Hoyos 
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

2015-02-26 Thread Nicolas George 
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 macos10 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

2015-02-26 Thread Derek Buitenhuis 
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

2015-02-26 Thread Derek Buitenhuis 
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