[FFmpeg-devel] [PATCH] Fix HTTP authentication problem for POST actions.

[jakob]

Hello,

After being busy with some other stuff, finally I had some time to go 
back to this fix. One important mystery was solved in the meantime: I 
was not able to retrieve the original bug reports on any of the 
ffmpeg-mail lists, simply because they were posted on the libav-dev list 
:P. Personally I had never heard about libav, but my colleague was under 
the impression that libav now is a library used by ffmpeg. After reading 
up a bit about libav/ffmpeg I'm wiser than that, yet the information is 
relevant and is here: 
http://lists.libav.org/pipermail/libav-tools/2013-August/000296.html.

After improving based on the remarks I intend to re-commit. I have a 
question though:
  * Is it desired to create a bug-report first for ffmpeg for having some 
(future) reference?

On 11.09.2013 16:34, Michael Niedermayer wrote:
> Hi
> 
> On Wed, Sep 11, 2013 at 03:05:48PM +0200, Jakob van Bethlehem wrote:
>> From: "J. van Bethlehem" <jakob at jet-stream.nl>
>> 
>> Upon executing HTTP POST requests, ffmpeg started the POST action
>> immediately after sending initial headers, therefore making it
>> impossible to properly deal with a 401 Authentication challenge from
>> the target server. This fix forces an initial empty POST request,
>> postponing POSTing data until after the proper HTTP authentication
>> header has been constructed
>> [ ... ]

> please dont make unrelated changes, instead post seperate patches,
> that is one for each self contained change
> "fix comments grammer and spelling"
> "factorize foobar"
> "fix this bug"
> "fix that bug"

I'll work on that.

>> @@ -607,7 +613,9 @@ static int http_connect(URLContext *h, const char 
>> *path, const char *local_path,
>>      if (!has_header(s->headers, "\r\nRange: ") && !post && (s->off > 
>> 0 || s->seekable == -1))
>>          len += av_strlcatf(headers + len, sizeof(headers) - len,
>>                             "Range: bytes=%"PRId64"-\r\n", s->off);
>> -
>> +    if (!has_header(s->headers, "\r\nHost: "))
>> +      len += av_strlcatf(headers + len, sizeof(headers) - len,
>> +                         "Host: %s\r\n", hoststr);
> 
> indention should be consistent within each file
Thought I had this covered: I'll take care of this.

> 
> 
>>      if (!has_header(s->headers, "\r\nConnection: ")) {
>>          if (s->multiple_requests) {
>>              len += av_strlcpy(headers + len, "Connection: 
>> keep-alive\r\n",
>> @@ -617,16 +625,6 @@ static int http_connect(URLContext *h, const char 
>> *path, const char *local_path,
>>                                sizeof(headers) - len);
>>          }
>>      }
>> -
>> -    if (!has_header(s->headers, "\r\nHost: "))
>> -        len += av_strlcatf(headers + len, sizeof(headers) - len,
>> -                           "Host: %s\r\n", hoststr);
> 
> why is this moved around ?

I believe there was a reason: I'll make sure to create separate commits 
to explain when re-committing (or put it back if there was no reason)

>> [ ... ]
> 
> the new code checks has_header() the old doesnt. why and it seems
> unrelated to the auth fix

Sounds like an oversight. I'll make sure also here to explain or remove 
when re-committing


> also please see tools/patcheck
Did not report relevant issues.

Sincerely,
Jakob van Bethlehem