[PATCHES] A bunch of http_proxy patches
Hi, I'm here again to make some noise, sorry guys. I have a few patches to propose in order to improve the http_proxy support. I used git (through git.mplayerhq.hu mirror) to stack patches, sorry I didn't have the fate to do that with SVN. Anyway, It should be compatible. Regards,
I already sent it in a previous mail: it allows login/password to be unescaped before base64 encode so auth with special characters now works. --- stream/http.c | 19 +++++++++++++++++++ 1 files changed, 19 insertions(+), 0 deletions(-) diff --git a/stream/http.c b/stream/http.c index 87dddc2..815992b 100644 --- a/stream/http.c +++ b/stream/http.c @@ -608,9 +608,26 @@ http_add_basic_authentication( HTTP_header_t *http_hdr, const char *username, co char *auth = NULL, *usr_pass = NULL, *b64_usr_pass = NULL; int encoded_len, pass_len=0, out_len; int res = -1; + char *username_decoded = NULL, *password_decoded = NULL; + if( http_hdr==NULL || username==NULL ) return -1; + username_decoded = malloc(strlen(username) + 1); + if (username_decoded == NULL) { + mp_msg(MSGT_NETWORK, MSGL_FATAL, "Memory allocation failed\n"); + goto out; + } + url_unescape_string(username_decoded, username); + username = username_decoded; + if( password!=NULL ) { + password_decoded = malloc(strlen(password) + 1); + if (password_decoded == NULL) { + mp_msg(MSGT_NETWORK, MSGL_FATAL, "Memory allocation failed\n"); + goto out; + } + url_unescape_string(password_decoded, password); + password = password_decoded; pass_len = strlen(password); } @@ -649,6 +666,8 @@ http_add_basic_authentication( HTTP_header_t *http_hdr, const char *username, co res = 0; out: + free(username_decoded); + free(password_decoded); free( usr_pass ); free( b64_usr_pass ); free( auth ); -- 1.7.3.1
On Mon, Oct 18, 2010 at 12:05:51PM +0200, Clément Bœsch wrote:
I already sent it in a previous mail: it allows login/password to be unescaped before base64 encode so auth with special characters now works.
Still no news about this first patch; it should not require a lot of skills to review this stuff, even if it's 8 patches. Can anyone do it, or at least start with this one please? Those bugs are really annoying and easy to fix… Regards, -- Clément B. Not sent from a jesusPhone.
On Sun, Oct 24, 2010 at 08:10:00PM +0200, Clément Bœsch wrote:
On Mon, Oct 18, 2010 at 12:05:51PM +0200, Clément Bœsch wrote:
I already sent it in a previous mail: it allows login/password to be unescaped before base64 encode so auth with special characters now works.
Still no news about this first patch; it should not require a lot of skills to review this stuff, even if it's 8 patches. Can anyone do it, or at least start with this one please? Those bugs are really annoying and easy to fix…
It's not at all easy to figure out if they actually fix more or break more... This first patch for example I don't think anyone who has a %-sign in the password will like. Does e.g. Firefox really try to unescape the password?
On Sun, Oct 24, 2010 at 08:31:06PM +0200, Reimar Döffinger wrote:
On Sun, Oct 24, 2010 at 08:10:00PM +0200, Clément Bœsch wrote:
On Mon, Oct 18, 2010 at 12:05:51PM +0200, Clément Bœsch wrote:
I already sent it in a previous mail: it allows login/password to be unescaped before base64 encode so auth with special characters now works.
Still no news about this first patch; it should not require a lot of skills to review this stuff, even if it's 8 patches. Can anyone do it, or at least start with this one please? Those bugs are really annoying and easy to fix…
It's not at all easy to figure out if they actually fix more or break more... This first patch for example I don't think anyone who has a %-sign in the password will like. Does e.g. Firefox really try to unescape the password?
The unescape is done because the URL is url-encoded in MPlayer (and so login/password). '%' seems to work: % ./mplayer 'http_proxy://login_x:a%b%c@localhost:8123/http://localhost:8000 [...] % polipo daemonise=false 'authCredentials="login_x:a%b%c"' Established listening socket on port 8123. % nc -v -l localhost -p 8000 Connection from 127.0.0.1:32831 GET / HTTP/1.1 Host: localhost:8000 User-Agent: MPlayer UNKNOWN-4.5.1 Icy-MetaData: 1 Connection: keep-alive -- Clément B. Not sent from a jesusPhone.
On Sun, Oct 24, 2010 at 09:07:50PM +0200, Clément Bœsch wrote:
On Sun, Oct 24, 2010 at 08:31:06PM +0200, Reimar Döffinger wrote:
On Sun, Oct 24, 2010 at 08:10:00PM +0200, Clément Bœsch wrote:
On Mon, Oct 18, 2010 at 12:05:51PM +0200, Clément Bœsch wrote:
I already sent it in a previous mail: it allows login/password to be unescaped before base64 encode so auth with special characters now works.
Still no news about this first patch; it should not require a lot of skills to review this stuff, even if it's 8 patches. Can anyone do it, or at least start with this one please? Those bugs are really annoying and easy to fix…
It's not at all easy to figure out if they actually fix more or break more... This first patch for example I don't think anyone who has a %-sign in the password will like. Does e.g. Firefox really try to unescape the password?
The unescape is done because the URL is url-encoded in MPlayer (and so login/password). '%' seems to work:
% ./mplayer 'http_proxy://login_x:a%b%c@localhost:8123/http://localhost:8000 [...]
Hm, ok, however I suspect that the whole escaping handling in MPlayer is rather broken so I am not sure if this is more a way of fixing things or if it's more kind of adding a bug to hide the other bug that will make other fixes more difficult...
On Sun, Oct 24, 2010 at 09:12:37PM +0200, Reimar Döffinger wrote:
On Sun, Oct 24, 2010 at 09:07:50PM +0200, Clément Bœsch wrote:
On Sun, Oct 24, 2010 at 08:31:06PM +0200, Reimar Döffinger wrote:
On Sun, Oct 24, 2010 at 08:10:00PM +0200, Clément Bœsch wrote:
On Mon, Oct 18, 2010 at 12:05:51PM +0200, Clément Bœsch wrote:
I already sent it in a previous mail: it allows login/password to be unescaped before base64 encode so auth with special characters now works.
Still no news about this first patch; it should not require a lot of skills to review this stuff, even if it's 8 patches. Can anyone do it, or at least start with this one please? Those bugs are really annoying and easy to fix…
It's not at all easy to figure out if they actually fix more or break more... This first patch for example I don't think anyone who has a %-sign in the password will like. Does e.g. Firefox really try to unescape the password?
The unescape is done because the URL is url-encoded in MPlayer (and so login/password). '%' seems to work:
% ./mplayer 'http_proxy://login_x:a%b%c@localhost:8123/http://localhost:8000 [...]
Hm, ok, however I suspect that the whole escaping handling in MPlayer is rather broken so I am not sure if this is more a way of fixing things or if it's more kind of adding a bug to hide the other bug that will make other fixes more difficult...
ok, then here is the updated patch but with the two "cosmetics" patches you requested to have before. -- Clément B. Not sent from a jesusPhone.
On Sun, Oct 24, 2010 at 09:40:00PM +0200, Clément Bœsch wrote:
On Sun, Oct 24, 2010 at 09:12:37PM +0200, Reimar Döffinger wrote:
On Sun, Oct 24, 2010 at 09:07:50PM +0200, Clément Bœsch wrote:
On Sun, Oct 24, 2010 at 08:31:06PM +0200, Reimar Döffinger wrote:
On Sun, Oct 24, 2010 at 08:10:00PM +0200, Clément Bœsch wrote:
On Mon, Oct 18, 2010 at 12:05:51PM +0200, Clément Bœsch wrote:
I already sent it in a previous mail: it allows login/password to be unescaped before base64 encode so auth with special characters now works.
Still no news about this first patch; it should not require a lot of skills to review this stuff, even if it's 8 patches. Can anyone do it, or at least start with this one please? Those bugs are really annoying and easy to fix…
It's not at all easy to figure out if they actually fix more or break more... This first patch for example I don't think anyone who has a %-sign in the password will like. Does e.g. Firefox really try to unescape the password?
The unescape is done because the URL is url-encoded in MPlayer (and so login/password). '%' seems to work:
% ./mplayer 'http_proxy://login_x:a%b%c@localhost:8123/http://localhost:8000 [...]
Hm, ok, however I suspect that the whole escaping handling in MPlayer is rather broken so I am not sure if this is more a way of fixing things or if it's more kind of adding a bug to hide the other bug that will make other fixes more difficult...
ok, then here is the updated patch but with the two "cosmetics" patches you requested to have before.
Ping? -- Clément B. Not sent from a jesusPhone.
On Wed, Nov 03, 2010 at 09:31:09PM +0100, Clément Bœsch wrote:
On Sun, Oct 24, 2010 at 09:40:00PM +0200, Clément Bœsch wrote:
ok, then here is the updated patch but with the two "cosmetics" patches you requested to have before.
Ping?
You have sent quite a number of patches, and while I still have comments on a few, a lot are already ok and for the others despite my comments/complaints I think they are better applied than forgotten. So while I expect you'll still show some patience for me to review, I'd suggest that you read DOCS/svn-howto.txt and if you are ok with write a gpg-encrypted email to Diego with your desired username and password and then commit any patches are ok'd (or of course if you can't find anyone to review in a reasonable time and it's well tested). Reimar
On Sat, Nov 06, 2010 at 11:16:25AM +0100, Reimar Döffinger wrote:
On Wed, Nov 03, 2010 at 09:31:09PM +0100, Clément Bœsch wrote:
On Sun, Oct 24, 2010 at 09:40:00PM +0200, Clément Bœsch wrote:
ok, then here is the updated patch but with the two "cosmetics" patches you requested to have before.
Ping?
You have sent quite a number of patches, and while I still have comments on a few, a lot are already ok and for the others despite my comments/complaints I think they are better applied than forgotten. So while I expect you'll still show some patience for me to review, I'd suggest that you read DOCS/svn-howto.txt and if you are ok with write a gpg-encrypted email to Diego with your desired username and password and then commit any patches are ok'd (or of course if you can't find anyone to review in a reasonable time and it's well tested).
I've read DOCS/tech/svn-howto.txt and I'm obviously ok with them and what you just said. Thank you, I'll send what you asked for in a minute to Diego.
Reimar
-- Clément B. Not sent from a jesusPhone.
On Sun, Oct 24, 2010 at 09:40:00PM +0200, Clément Bœsch wrote:
On Sun, Oct 24, 2010 at 09:12:37PM +0200, Reimar Döffinger wrote:
On Sun, Oct 24, 2010 at 09:07:50PM +0200, Clément Bœsch wrote:
On Sun, Oct 24, 2010 at 08:31:06PM +0200, Reimar Döffinger wrote:
On Sun, Oct 24, 2010 at 08:10:00PM +0200, Clément Bœsch wrote:
On Mon, Oct 18, 2010 at 12:05:51PM +0200, Clément Bœsch wrote:
I already sent it in a previous mail: it allows login/password to be unescaped before base64 encode so auth with special characters now works.
Still no news about this first patch; it should not require a lot of skills to review this stuff, even if it's 8 patches. Can anyone do it, or at least start with this one please? Those bugs are really annoying and easy to fix…
It's not at all easy to figure out if they actually fix more or break more... This first patch for example I don't think anyone who has a %-sign in the password will like. Does e.g. Firefox really try to unescape the password?
The unescape is done because the URL is url-encoded in MPlayer (and so login/password). '%' seems to work:
% ./mplayer 'http_proxy://login_x:a%b%c@localhost:8123/http://localhost:8000 [...]
Hm, ok, however I suspect that the whole escaping handling in MPlayer is rather broken so I am not sure if this is more a way of fixing things or if it's more kind of adding a bug to hide the other bug that will make other fixes more difficult...
ok, then here is the updated patch but with the two "cosmetics" patches you requested to have before.
Is it OK to commit at least the first two ones (allocation string and 256 to sizeof) so we can focus on the real issue? -- Clément B. Not sent from a jesusPhone.
On Sun, Nov 07, 2010 at 09:49:57PM +0100, Clément Bœsch wrote:
On Sun, Oct 24, 2010 at 09:40:00PM +0200, Clément Bœsch wrote:
ok, then here is the updated patch but with the two "cosmetics" patches you requested to have before.
Is it OK to commit at least the first two ones (allocation string and 256 to sizeof) so we can focus on the real issue?
Yes, I said ok to them almost right at the start :-)
On Mon, Oct 18, 2010 at 12:05:51PM +0200, Clément Bœsch wrote:
I already sent it in a previous mail: it allows login/password to be unescaped before base64 encode so auth with special characters now works.
Al'right, a new version of this patch: while the form mplayer 'http://u:p@host' was url encoded, mplayer 'http://host' -user u -passwd p wasn't. Now it's fixed. Reimar, you also mentionned the url escaping may be broken, what was the issue? Anyway, is this version fine? -- Clément B. Not sent from a jesusPhone.
On Sun, Nov 07, 2010 at 11:31:50PM +0100, Clément Bœsch wrote:
On Mon, Oct 18, 2010 at 12:05:51PM +0200, Clément Bœsch wrote:
I already sent it in a previous mail: it allows login/password to be unescaped before base64 encode so auth with special characters now works.
Al'right, a new version of this patch: while the form
mplayer 'http://u:p@host'
was url encoded,
mplayer 'http://host' -user u -passwd p
wasn't.
Now it's fixed.
Reimar, you also mentionned the url escaping may be broken, what was the issue?
Anyway, is this version fine?
If anyone has something against this, speak now or I'll commit this one it in the next days. -- Clément B. Not sent from a jesusPhone.
On Wed, Nov 10, 2010 at 08:02:14PM +0100, Clément Bœsch wrote:
On Sun, Nov 07, 2010 at 11:31:50PM +0100, Clément Bœsch wrote:
On Mon, Oct 18, 2010 at 12:05:51PM +0200, Clément Bœsch wrote:
I already sent it in a previous mail: it allows login/password to be unescaped before base64 encode so auth with special characters now works.
Al'right, a new version of this patch: while the form
mplayer 'http://u:p@host'
was url encoded,
mplayer 'http://host' -user u -passwd p
wasn't.
Now it's fixed.
Reimar, you also mentionned the url escaping may be broken, what was the issue?
Anyway, is this version fine?
If anyone has something against this, speak now or I'll commit this one it in the next days.
I have the suspicion that this is wildly escaping and unescaping until it just happens to work without any real concept. Why does url_new not just apply escaping only to the path part? I don't think escaping is supposed to be applied to anything else.
On Wed, Nov 10, 2010 at 08:35:46PM +0100, Reimar Döffinger wrote:
On Wed, Nov 10, 2010 at 08:02:14PM +0100, Clément Bœsch wrote:
On Sun, Nov 07, 2010 at 11:31:50PM +0100, Clément Bœsch wrote:
On Mon, Oct 18, 2010 at 12:05:51PM +0200, Clément Bœsch wrote:
I already sent it in a previous mail: it allows login/password to be unescaped before base64 encode so auth with special characters now works.
Al'right, a new version of this patch: while the form
mplayer 'http://u:p@host'
was url encoded,
mplayer 'http://host' -user u -passwd p
wasn't.
Now it's fixed.
Reimar, you also mentionned the url escaping may be broken, what was the issue?
Anyway, is this version fine?
If anyone has something against this, speak now or I'll commit this one it in the next days.
I have the suspicion that this is wildly escaping and unescaping until it just happens to work without any real concept. Why does url_new not just apply escaping only to the path part? I don't think escaping is supposed to be applied to anything else.
Note that this would also allow getting rid of the IPv6-hack in the escaping code.
On Wed, Nov 10, 2010 at 08:35:46PM +0100, Reimar Döffinger wrote:
On Wed, Nov 10, 2010 at 08:02:14PM +0100, Clément Bœsch wrote:
On Sun, Nov 07, 2010 at 11:31:50PM +0100, Clément Bœsch wrote:
On Mon, Oct 18, 2010 at 12:05:51PM +0200, Clément Bœsch wrote:
I already sent it in a previous mail: it allows login/password to be unescaped before base64 encode so auth with special characters now works.
Al'right, a new version of this patch: while the form
mplayer 'http://u:p@host'
was url encoded,
mplayer 'http://host' -user u -passwd p
wasn't.
Now it's fixed.
Reimar, you also mentionned the url escaping may be broken, what was the issue?
Anyway, is this version fine?
If anyone has something against this, speak now or I'll commit this one it in the next days.
I have the suspicion that this is wildly escaping and unescaping until it just happens to work without any real concept. Why does url_new not just apply escaping only to the path part?
As I said the first time, I started to make a patch to update the escaping, but it much more complicated and risky; imagine you simply have to parse: http_proxy://foo:bar@host:4321/http://xxx:yyy@proxy:1234. There is some cases not easy to handle.
I don't think escaping is supposed to be applied to anything else.
We could also have users who try to make special character by urlencoding themselves the password (special char not easy to escape with the shell, or simply break url parsing in MPlayer because of ':' or '@' in it). MPlayer urlencode won't change the string, but the http auth code will be able to decode it. -- Clément B. Not sent from a jesusPhone.
On Wed, Nov 10, 2010 at 08:50:28PM +0100, Clément Bœsch wrote:
On Wed, Nov 10, 2010 at 08:35:46PM +0100, Reimar Döffinger wrote:
On Wed, Nov 10, 2010 at 08:02:14PM +0100, Clément Bœsch wrote:
On Sun, Nov 07, 2010 at 11:31:50PM +0100, Clément Bœsch wrote:
On Mon, Oct 18, 2010 at 12:05:51PM +0200, Clément Bœsch wrote:
I already sent it in a previous mail: it allows login/password to be unescaped before base64 encode so auth with special characters now works.
Al'right, a new version of this patch: while the form
mplayer 'http://u:p@host'
was url encoded,
mplayer 'http://host' -user u -passwd p
wasn't.
Now it's fixed.
Reimar, you also mentionned the url escaping may be broken, what was the issue?
Anyway, is this version fine?
If anyone has something against this, speak now or I'll commit this one it in the next days.
I have the suspicion that this is wildly escaping and unescaping until it just happens to work without any real concept. Why does url_new not just apply escaping only to the path part?
As I said the first time, I started to make a patch to update the escaping, but it much more complicated and risky; imagine you simply have to parse: http_proxy://foo:bar@host:4321/http://xxx:yyy@proxy:1234. There is some cases not easy to handle.
And what about unescaping in url_new when username/password is assigned?
I don't think escaping is supposed to be applied to anything else.
We could also have users who try to make special character by urlencoding themselves the password (special char not easy to escape with the shell, or simply break url parsing in MPlayer because of ':' or '@' in it). MPlayer urlencode won't change the string, but the http auth code will be able to decode it.
Well, thinking more about it I have the suspicion that your patch will actually break e.g. %20 as password, it well end up using a single space as password (unless you pre-escape it of course)... Doing it in url_new would at least allow the other method of specifying it... I'd actually be quite curious what webbrowser do in such a case.
On Wed, Nov 10, 2010 at 09:20:08PM +0100, Reimar Döffinger wrote:
On Wed, Nov 10, 2010 at 08:50:28PM +0100, Clément Bœsch wrote:
On Wed, Nov 10, 2010 at 08:35:46PM +0100, Reimar Döffinger wrote:
I have the suspicion that this is wildly escaping and unescaping until it just happens to work without any real concept. Why does url_new not just apply escaping only to the path part?
As I said the first time, I started to make a patch to update the escaping, but it much more complicated and risky; imagine you simply have to parse: http_proxy://foo:bar@host:4321/http://xxx:yyy@proxy:1234. There is some cases not easy to handle.
And what about unescaping in url_new when username/password is assigned?
Yes, this seems actually to be a better solution.
I don't think escaping is supposed to be applied to anything else.
We could also have users who try to make special character by urlencoding themselves the password (special char not easy to escape with the shell, or simply break url parsing in MPlayer because of ':' or '@' in it). MPlayer urlencode won't change the string, but the http auth code will be able to decode it.
Well, thinking more about it I have the suspicion that your patch will actually break e.g. %20 as password, it well end up using a single space as password (unless you pre-escape it of course)... Doing it in url_new would at least allow the other method of specifying it... I'd actually be quite curious what webbrowser do in such a case.
MPlayer is closer to curl than a browser in term of use, so I just compared with it, and here is the behaviour: curl 'http://usr:%20@localhost:8000' => 'dXNyOiA=' => 'usr: ' curl 'http://locahost:8000' -u 'usr:%20' => 'dXNyOiUyMA==' => 'usr:%20' So indeed, unescaping in url_new seems to be the right thing to do. Patch attached. If you want to test, note that the form mplayer $url -user $usr -passwd $pass will not send the auth at first, it waits for the server to ask for an Auth Required signal. -- Clément B. Not sent from a jesusPhone.
On Thu, Nov 11, 2010 at 01:41:39PM +0100, Clément Bœsch wrote:
And what about unescaping in url_new when username/password is assigned?
Yes, this seems actually to be a better solution.
Why not like patch below?
I don't think escaping is supposed to be applied to anything else.
We could also have users who try to make special character by urlencoding themselves the password (special char not easy to escape with the shell, or simply break url parsing in MPlayer because of ':' or '@' in it). MPlayer urlencode won't change the string, but the http auth code will be able to decode it.
Well, thinking more about it I have the suspicion that your patch will actually break e.g. %20 as password, it well end up using a single space as password (unless you pre-escape it of course)... Doing it in url_new would at least allow the other method of specifying it... I'd actually be quite curious what webbrowser do in such a case.
MPlayer is closer to curl than a browser in term of use, so I just compared with it, and here is the behaviour:
curl 'http://usr:%20@localhost:8000' => 'dXNyOiA=' => 'usr: ' curl 'http://locahost:8000' -u 'usr:%20' => 'dXNyOiUyMA==' => 'usr:%20'
So indeed, unescaping in url_new seems to be the right thing to do. Patch attached.
Very nice test, that gives me a lot more confidence that we are doing something sane. Index: url.c =================================================================== --- url.c (revision 32620) +++ url.c (working copy) @@ -153,7 +153,9 @@ } strncpy( Curl->password, ptr3+1, len2); Curl->password[len2]='\0'; + url_unescape_string(Curl->password, Curl->password); } + url_unescape_string(Curl->username, Curl->username); ptr1 = ptr2+1; pos1 = ptr1-escfilename; } @@ -252,7 +254,8 @@ /* Replace escape sequences in an URL (or a part of an URL) */ -/* works like strcpy(), but without return argument */ +/* works like strcpy(), but without return argument, + except that outbuf == inbuf is allowed */ void url_unescape_string(char *outbuf, const char *inbuf) {
On Thu, Nov 11, 2010 at 08:36:07PM +0100, Reimar Döffinger wrote:
On Thu, Nov 11, 2010 at 01:41:39PM +0100, Clément Bœsch wrote:
And what about unescaping in url_new when username/password is assigned?
Yes, this seems actually to be a better solution.
Why not like patch below?
Oh… I feel stupid :) Yes please, commit this version.
I don't think escaping is supposed to be applied to anything else.
We could also have users who try to make special character by urlencoding themselves the password (special char not easy to escape with the shell, or simply break url parsing in MPlayer because of ':' or '@' in it). MPlayer urlencode won't change the string, but the http auth code will be able to decode it.
Well, thinking more about it I have the suspicion that your patch will actually break e.g. %20 as password, it well end up using a single space as password (unless you pre-escape it of course)... Doing it in url_new would at least allow the other method of specifying it... I'd actually be quite curious what webbrowser do in such a case.
MPlayer is closer to curl than a browser in term of use, so I just compared with it, and here is the behaviour:
curl 'http://usr:%20@localhost:8000' => 'dXNyOiA=' => 'usr: ' curl 'http://locahost:8000' -u 'usr:%20' => 'dXNyOiUyMA==' => 'usr:%20'
So indeed, unescaping in url_new seems to be the right thing to do. Patch attached.
Very nice test, that gives me a lot more confidence that we are doing something sane.
Also note urlencoding the password for an http proxy with MPlayer will be the only solution to specify a "complex" password since there is no -proxy-user and -proxy-passwd. But well, it will be possible. Maybe more than an example in the man pages will be needed. So please, commit this so I can go ahead for the next issue. Anyway, thanks for your implication :)
Index: url.c =================================================================== --- url.c (revision 32620) +++ url.c (working copy) @@ -153,7 +153,9 @@ } strncpy( Curl->password, ptr3+1, len2); Curl->password[len2]='\0'; + url_unescape_string(Curl->password, Curl->password); } + url_unescape_string(Curl->username, Curl->username); ptr1 = ptr2+1; pos1 = ptr1-escfilename; } @@ -252,7 +254,8 @@
/* Replace escape sequences in an URL (or a part of an URL) */ -/* works like strcpy(), but without return argument */ +/* works like strcpy(), but without return argument, + except that outbuf == inbuf is allowed */ void url_unescape_string(char *outbuf, const char *inbuf) { _______________________________________________ MPlayer-dev-eng mailing list MPlayer-dev-eng@mplayerhq.hu https://lists.mplayerhq.hu/mailman/listinfo/mplayer-dev-eng
-- Clément B. Not sent from a jesusPhone.
On Thu, Nov 11, 2010 at 11:05:03PM +0100, Clément Bœsch wrote:
On Thu, Nov 11, 2010 at 08:36:07PM +0100, Reimar Döffinger wrote:
On Thu, Nov 11, 2010 at 01:41:39PM +0100, Clément Bœsch wrote:
And what about unescaping in url_new when username/password is assigned?
Yes, this seems actually to be a better solution.
Why not like patch below?
Oh… I feel stupid :)
Yes please, commit this version.
Done. I also noticed one thing you haven't set a patch for: http_proxy will not work if the "main" URL uses an IPv6 address.
When using http_proxy, the authentication header is not correctly set: it uses the same "Authorization" header as the http one. Here is a patch to adapt the behaviour with proxies (thanks to Louis Opter <kalessin@kalessin.fr> helping me pointing out the issue). --- stream/http.c | 18 ++++++++++++++---- stream/http.h | 1 + stream/network.c | 5 ++++- 3 files changed, 19 insertions(+), 5 deletions(-) diff --git a/stream/http.c b/stream/http.c index 815992b..c5f1b12 100644 --- a/stream/http.c +++ b/stream/http.c @@ -603,8 +603,8 @@ http_set_uri( HTTP_header_t *http_hdr, const char *uri ) { strcpy( http_hdr->uri, uri ); } -int -http_add_basic_authentication( HTTP_header_t *http_hdr, const char *username, const char *password ) { +static int +http_add_authentication( HTTP_header_t *http_hdr, const char *username, const char *password, const char *auth_str ) { char *auth = NULL, *usr_pass = NULL, *b64_usr_pass = NULL; int encoded_len, pass_len=0, out_len; int res = -1; @@ -655,13 +655,13 @@ http_add_basic_authentication( HTTP_header_t *http_hdr, const char *username, co b64_usr_pass[out_len]='\0'; - auth = malloc(encoded_len+22); + auth = malloc(encoded_len + strlen(auth_str) + 1); if( auth==NULL ) { mp_msg(MSGT_NETWORK,MSGL_FATAL,"Memory allocation failed\n"); goto out; } - sprintf( auth, "Authorization: Basic %s", b64_usr_pass); + sprintf( auth, "%s%s", auth_str, b64_usr_pass); http_set_field( http_hdr, auth ); res = 0; @@ -675,6 +675,16 @@ out: return res; } +int +http_add_basic_authentication( HTTP_header_t *http_hdr, const char *username, const char *password ) { + return http_add_authentication(http_hdr, username, password, "Authorization: Basic "); +} + +int +http_add_basic_proxy_authentication( HTTP_header_t *http_hdr, const char *username, const char *password ) { + return http_add_authentication(http_hdr, username, password, "Proxy-Authorization: Basic "); +} + void http_debug_hdr( HTTP_header_t *http_hdr ) { HTTP_field_t *field; diff --git a/stream/http.h b/stream/http.h index c52b9c8..d24ec45 100644 --- a/stream/http.h +++ b/stream/http.h @@ -63,6 +63,7 @@ void http_set_field( HTTP_header_t *http_hdr, const char *field_name ); void http_set_method( HTTP_header_t *http_hdr, const char *method ); void http_set_uri( HTTP_header_t *http_hdr, const char *uri ); int http_add_basic_authentication( HTTP_header_t *http_hdr, const char *username, const char *password ); +int http_add_basic_proxy_authentication( HTTP_header_t *http_hdr, const char *username, const char *password ); void http_debug_hdr( HTTP_header_t *http_hdr ); diff --git a/stream/network.c b/stream/network.c index b065ece..9f81609 100644 --- a/stream/network.c +++ b/stream/network.c @@ -254,7 +254,10 @@ http_send_request( URL_t *url, off_t pos ) { if (network_cookies_enabled) cookies_set( http_hdr, server_url->hostname, server_url->url ); http_set_field( http_hdr, "Connection: close"); - http_add_basic_authentication( http_hdr, url->username, url->password ); + if (proxy) + http_add_basic_proxy_authentication( http_hdr, url->username, url->password ); + else + http_add_basic_authentication( http_hdr, url->username, url->password ); if( http_build_request( http_hdr )==NULL ) { goto err_out; } -- 1.7.3.1
On Mon, Oct 18, 2010 at 12:05:52PM +0200, Clément Bœsch wrote:
+int +http_add_basic_authentication( HTTP_header_t *http_hdr, const char *username, const char *password ) { + return http_add_authentication(http_hdr, username, password, "Authorization: Basic "); +} + +int +http_add_basic_proxy_authentication( HTTP_header_t *http_hdr, const char *username, const char *password ) { + return http_add_authentication(http_hdr, username, password, "Proxy-Authorization: Basic "); +}
It might slightly nicer to only have "Authorization" and "Proxy-Authorization" as strings, seems likely to make future changes simpler.
On Sun, Oct 24, 2010 at 08:40:08PM +0200, Reimar Döffinger wrote:
On Mon, Oct 18, 2010 at 12:05:52PM +0200, Clément Bœsch wrote:
+int +http_add_basic_authentication( HTTP_header_t *http_hdr, const char *username, const char *password ) { + return http_add_authentication(http_hdr, username, password, "Authorization: Basic "); +} + +int +http_add_basic_proxy_authentication( HTTP_header_t *http_hdr, const char *username, const char *password ) { + return http_add_authentication(http_hdr, username, password, "Proxy-Authorization: Basic "); +}
It might slightly nicer to only have "Authorization" and "Proxy-Authorization" as strings, seems likely to make future changes simpler.
Done. -- Clément B. Not sent from a jesusPhone.
On Sun, Oct 24, 2010 at 09:14:17PM +0200, Clément Bœsch wrote:
diff --git a/stream/network.c b/stream/network.c index 4853f87..7de834f 100644 --- a/stream/network.c +++ b/stream/network.c @@ -254,7 +254,10 @@ http_send_request( URL_t *url, off_t pos ) { if (network_cookies_enabled) cookies_set( http_hdr, server_url->hostname, server_url->url );
http_set_field( http_hdr, "Connection: close"); - http_add_basic_authentication( http_hdr, url->username, url->password ); + if (proxy) + http_add_basic_proxy_authentication( http_hdr, url->username, url->password ); + else + http_add_basic_authentication( http_hdr, url->username, url->password );
As pointed out in comment to patch 3, this is possibly wrong, but at least suboptimal. I'd actually use a separate patch that just changes url -> server_url in http_add_basic_authentication, unless there's any reason why this would break anything?
On Sat, Nov 06, 2010 at 11:09:23AM +0100, Reimar Döffinger wrote:
On Sun, Oct 24, 2010 at 09:14:17PM +0200, Clément Bœsch wrote:
diff --git a/stream/network.c b/stream/network.c index 4853f87..7de834f 100644 --- a/stream/network.c +++ b/stream/network.c @@ -254,7 +254,10 @@ http_send_request( URL_t *url, off_t pos ) { if (network_cookies_enabled) cookies_set( http_hdr, server_url->hostname, server_url->url );
http_set_field( http_hdr, "Connection: close"); - http_add_basic_authentication( http_hdr, url->username, url->password ); + if (proxy) + http_add_basic_proxy_authentication( http_hdr, url->username, url->password ); + else + http_add_basic_authentication( http_hdr, url->username, url->password );
As pointed out in comment to patch 3, this is possibly wrong, but at least suboptimal. I'd actually use a separate patch that just changes url -> server_url in http_add_basic_authentication, unless there's any reason why this would break anything?
No it shouldn't break anything, and after a few tests it doesn't. Also, it makes the Proxy-Authorization just missing instead of apparently broken. Patches re-attached. The next step is to make both authentications working in the same time (patch 3 and +). -- Clément B. Not sent from a jesusPhone.
On Fri, Nov 12, 2010 at 10:40:57AM +0100, Clément Bœsch wrote:
On Sat, Nov 06, 2010 at 11:09:23AM +0100, Reimar Döffinger wrote:
On Sun, Oct 24, 2010 at 09:14:17PM +0200, Clément Bœsch wrote:
diff --git a/stream/network.c b/stream/network.c index 4853f87..7de834f 100644 --- a/stream/network.c +++ b/stream/network.c @@ -254,7 +254,10 @@ http_send_request( URL_t *url, off_t pos ) { if (network_cookies_enabled) cookies_set( http_hdr, server_url->hostname, server_url->url );
http_set_field( http_hdr, "Connection: close"); - http_add_basic_authentication( http_hdr, url->username, url->password ); + if (proxy) + http_add_basic_proxy_authentication( http_hdr, url->username, url->password ); + else + http_add_basic_authentication( http_hdr, url->username, url->password );
As pointed out in comment to patch 3, this is possibly wrong, but at least suboptimal. I'd actually use a separate patch that just changes url -> server_url in http_add_basic_authentication, unless there's any reason why this would break anything?
No it shouldn't break anything, and after a few tests it doesn't. Also, it makes the Proxy-Authorization just missing instead of apparently broken.
Patches re-attached. The next step is to make both authentications working in the same time (patch 3 and +).
If no one says anything about those two patches in the 3 next days, i'll assume they are okay and commit them to go ahead. -- Clément B. Not sent from a jesusPhone.
On Fri, Nov 12, 2010 at 10:40:57AM +0100, Clément Bœsch wrote:
@@ -637,13 +637,13 @@ http_add_basic_authentication( HTTP_header_t *http_hdr, const char *username, co
b64_usr_pass[out_len]='\0';
- auth = malloc(encoded_len+22); + auth = malloc(encoded_len + strlen(auth_str) + sizeof(": Basic ")); if( auth==NULL ) { mp_msg(MSGT_NETWORK,MSGL_FATAL,MSGTR_MemAllocFailed); goto out; }
- sprintf( auth, "Authorization: Basic %s", b64_usr_pass); + sprintf( auth, "%s: Basic %s", auth_str, b64_usr_pass);
I think their ok, though I think personally I'd feel slightly more comfortable if it was something like
buffer_len = encoded_len + 100; // arbitrary, large enough number malloc(buffer_len); snprintf(, buffer_len, ....);
While hard-coded is not that great, it avoids wasting CPU time on strlen, and also the duplicated ": Basic " string is a risk, and this way at least ensure there'll never be a buffer overflow no matter what (not having to thing about integer overflows, someone changing only one of those strings, ...).
On Tue, Nov 16, 2010 at 09:05:24PM +0100, Reimar Döffinger wrote:
On Fri, Nov 12, 2010 at 10:40:57AM +0100, Clément Bœsch wrote:
@@ -637,13 +637,13 @@ http_add_basic_authentication( HTTP_header_t *http_hdr, const char *username, co
b64_usr_pass[out_len]='\0';
- auth = malloc(encoded_len+22); + auth = malloc(encoded_len + strlen(auth_str) + sizeof(": Basic ")); if( auth==NULL ) { mp_msg(MSGT_NETWORK,MSGL_FATAL,MSGTR_MemAllocFailed); goto out; }
- sprintf( auth, "Authorization: Basic %s", b64_usr_pass); + sprintf( auth, "%s: Basic %s", auth_str, b64_usr_pass);
I think their ok, though I think personally I'd feel slightly more comfortable if it was something like
buffer_len = encoded_len + 100; // arbitrary, large enough number malloc(buffer_len); snprintf(, buffer_len, ....);
While hard-coded is not that great, it avoids wasting CPU time on strlen, and also the duplicated ": Basic " string is a risk, and this way at least ensure there'll never be a buffer overflow no matter what (not having to thing about integer overflows, someone changing only one of those strings, ...).
The CPU time on strlen in this case is imo totally inappropriate, however I agree with you arguments about the duplicated string, so what about the attached patch? -- Clément B. Not sent from a jesusPhone.
On Tue, Nov 16, 2010 at 09:53:06PM +0100, Clément Bœsch wrote:
On Tue, Nov 16, 2010 at 09:05:24PM +0100, Reimar Döffinger wrote:
On Fri, Nov 12, 2010 at 10:40:57AM +0100, Clément Bœsch wrote:
@@ -637,13 +637,13 @@ http_add_basic_authentication( HTTP_header_t *http_hdr, const char *username, co
b64_usr_pass[out_len]='\0';
- auth = malloc(encoded_len+22); + auth = malloc(encoded_len + strlen(auth_str) + sizeof(": Basic ")); if( auth==NULL ) { mp_msg(MSGT_NETWORK,MSGL_FATAL,MSGTR_MemAllocFailed); goto out; }
- sprintf( auth, "Authorization: Basic %s", b64_usr_pass); + sprintf( auth, "%s: Basic %s", auth_str, b64_usr_pass);
I think their ok, though I think personally I'd feel slightly more comfortable if it was something like
buffer_len = encoded_len + 100; // arbitrary, large enough number malloc(buffer_len); snprintf(, buffer_len, ....);
While hard-coded is not that great, it avoids wasting CPU time on strlen, and also the duplicated ": Basic " string is a risk, and this way at least ensure there'll never be a buffer overflow no matter what (not having to thing about integer overflows, someone changing only one of those strings, ...).
The CPU time on strlen in this case is imo totally inappropriate, however I agree with you arguments about the duplicated string, so what about the attached patch?
Well, while it's very unlikely to be a _new_ issue with this patch, to review it properly I'd still have to check that the addition will not cause an integer overflow. And the CPU (and honestly more code complexity) is something I just consider because, honestly, I can't see much of a point in using a "100 % correct" number.
On Tue, Nov 16, 2010 at 10:12:58PM +0100, Reimar Döffinger wrote:
On Tue, Nov 16, 2010 at 09:53:06PM +0100, Clément Bœsch wrote:
On Tue, Nov 16, 2010 at 09:05:24PM +0100, Reimar Döffinger wrote:
On Fri, Nov 12, 2010 at 10:40:57AM +0100, Clément Bœsch wrote:
@@ -637,13 +637,13 @@ http_add_basic_authentication( HTTP_header_t *http_hdr, const char *username, co
b64_usr_pass[out_len]='\0';
- auth = malloc(encoded_len+22); + auth = malloc(encoded_len + strlen(auth_str) + sizeof(": Basic ")); if( auth==NULL ) { mp_msg(MSGT_NETWORK,MSGL_FATAL,MSGTR_MemAllocFailed); goto out; }
- sprintf( auth, "Authorization: Basic %s", b64_usr_pass); + sprintf( auth, "%s: Basic %s", auth_str, b64_usr_pass);
I think their ok, though I think personally I'd feel slightly more comfortable if it was something like
buffer_len = encoded_len + 100; // arbitrary, large enough number malloc(buffer_len); snprintf(, buffer_len, ....);
While hard-coded is not that great, it avoids wasting CPU time on strlen, and also the duplicated ": Basic " string is a risk, and this way at least ensure there'll never be a buffer overflow no matter what (not having to thing about integer overflows, someone changing only one of those strings, ...).
The CPU time on strlen in this case is imo totally inappropriate, however I agree with you arguments about the duplicated string, so what about the attached patch?
Well, while it's very unlikely to be a _new_ issue with this patch, to review it properly I'd still have to check that the addition will not cause an integer overflow.
Changing encoded_len type from int to size_t may help?
And the CPU (and honestly more code complexity) is something I just consider because, honestly, I can't see much of a point in using a "100 % correct" number.
Not sure I get what you mean. Anyway, I still find the allocation procedure more appropriated, and I just updated the current code. -- Clément B. Not sent from a jesusPhone.
On Wed, Nov 17, 2010 at 12:21:20AM +0100, Clément Bœsch wrote:
On Tue, Nov 16, 2010 at 10:12:58PM +0100, Reimar Döffinger wrote:
Well, while it's very unlikely to be a _new_ issue with this patch, to review it properly I'd still have to check that the addition will not cause an integer overflow.
Changing encoded_len type from int to size_t may help?
Maybe. Why not just use the solution that does not require thinking?
And the CPU (and honestly more code complexity) is something I just consider because, honestly, I can't see much of a point in using a "100 % correct" number.
Not sure I get what you mean. Anyway, I still find the allocation procedure more appropriated, and I just updated the current code.
The value is wrong anyway due to using encoded_len instead of out_len. I am say that calculating the precise value to allocate is a waste of developing, review and maintainance effort when all you need is a value >=. In addition having an extra malloc there is quite stupid, it should be something like encoded_len += 100; b64_usr_pass = malloc(encoded_len); av_strlcpy(b64_usr_pass, authstr); av_strlcat... base64_encode but maybe it's better to just leave that for later.
On Wed, Nov 17, 2010 at 08:23:21AM +0100, Reimar Döffinger wrote:
On Wed, Nov 17, 2010 at 12:21:20AM +0100, Clément Bœsch wrote:
On Tue, Nov 16, 2010 at 10:12:58PM +0100, Reimar Döffinger wrote:
Well, while it's very unlikely to be a _new_ issue with this patch, to review it properly I'd still have to check that the addition will not cause an integer overflow.
Changing encoded_len type from int to size_t may help?
Maybe. Why not just use the solution that does not require thinking?
It's not really a matter of thinkink, I was just changing the current code without changing the allocation procedure because it's not the purpose of the patch.
And the CPU (and honestly more code complexity) is something I just consider because, honestly, I can't see much of a point in using a "100 % correct" number.
Not sure I get what you mean. Anyway, I still find the allocation procedure more appropriated, and I just updated the current code.
The value is wrong anyway due to using encoded_len instead of out_len. I am say that calculating the precise value to allocate is a waste of developing, review and maintainance effort when all you need is a value >=. In addition having an extra malloc there is quite stupid, it should be something like encoded_len += 100; b64_usr_pass = malloc(encoded_len); av_strlcpy(b64_usr_pass, authstr); av_strlcat... base64_encode but maybe it's better to just leave that for later.
I have a pending patch (not yet sent) to change the current base64_encode with call to FFmpeg one, and that code will need to be changed so yes, I think it's better to leave it for later. -- Clément B. Not sent from a jesusPhone.
On Tue, Nov 16, 2010 at 09:05:24PM +0100, Reimar Döffinger wrote:
On Fri, Nov 12, 2010 at 10:40:57AM +0100, Clément Bœsch wrote:
@@ -637,13 +637,13 @@ http_add_basic_authentication( HTTP_header_t *http_hdr, const char *username, co
b64_usr_pass[out_len]='\0';
- auth = malloc(encoded_len+22); + auth = malloc(encoded_len + strlen(auth_str) + sizeof(": Basic ")); if( auth==NULL ) { mp_msg(MSGT_NETWORK,MSGL_FATAL,MSGTR_MemAllocFailed); goto out; }
- sprintf( auth, "Authorization: Basic %s", b64_usr_pass); + sprintf( auth, "%s: Basic %s", auth_str, b64_usr_pass);
I think their ok, though I think personally I'd feel slightly more comfortable if it was something like
buffer_len = encoded_len + 100; // arbitrary, large enough number malloc(buffer_len); snprintf(, buffer_len, ....);
While hard-coded is not that great, it avoids wasting CPU time on strlen, and also the duplicated ": Basic " string is a risk, and this way at least ensure there'll never be a buffer overflow no matter what (not having to thing about integer overflows, someone changing only one of those strings, ...).
Alright, so new version with this arbitrary length attached. If it's fine this time, I'll make a last series of tests tonight and commit the two patches tomorrow. -- Clément B. Not sent from a jesusPhone.
On Wed, Nov 17, 2010 at 09:45:34AM +0100, Clément Bœsch wrote:
On Tue, Nov 16, 2010 at 09:05:24PM +0100, Reimar Döffinger wrote:
On Fri, Nov 12, 2010 at 10:40:57AM +0100, Clément Bœsch wrote:
@@ -637,13 +637,13 @@ http_add_basic_authentication( HTTP_header_t *http_hdr, const char *username, co
b64_usr_pass[out_len]='\0';
- auth = malloc(encoded_len+22); + auth = malloc(encoded_len + strlen(auth_str) + sizeof(": Basic ")); if( auth==NULL ) { mp_msg(MSGT_NETWORK,MSGL_FATAL,MSGTR_MemAllocFailed); goto out; }
- sprintf( auth, "Authorization: Basic %s", b64_usr_pass); + sprintf( auth, "%s: Basic %s", auth_str, b64_usr_pass);
I think their ok, though I think personally I'd feel slightly more comfortable if it was something like
buffer_len = encoded_len + 100; // arbitrary, large enough number malloc(buffer_len); snprintf(, buffer_len, ....);
While hard-coded is not that great, it avoids wasting CPU time on strlen, and also the duplicated ": Basic " string is a risk, and this way at least ensure there'll never be a buffer overflow no matter what (not having to thing about integer overflows, someone changing only one of those strings, ...).
Alright, so new version with this arbitrary length attached. If it's fine this time, I'll make a last series of tests tonight and commit the two patches tomorrow.
Both applied. -- Clément B. Not sent from a jesusPhone.
Currently, http_proxy:// does not forward server auth correcly, here is a fix. --- stream/network.c | 13 ++++++++++--- 1 files changed, 10 insertions(+), 3 deletions(-) diff --git a/stream/network.c b/stream/network.c index 9f81609..1ec1220 100644 --- a/stream/network.c +++ b/stream/network.c @@ -209,7 +209,11 @@ http_send_request( URL_t *url, off_t pos ) { mp_msg(MSGT_NETWORK, MSGL_ERR, "Invalid URL '%s' to proxify\n", url->file+1); goto err_out; } - http_set_uri( http_hdr, server_url->url ); + snprintf(str, 256, "http://%s:%d%s", + server_url->hostname, + server_url->port ? server_url->port : 80, + server_url->file); + http_set_uri( http_hdr, str ); } else { server_url = url; http_set_uri( http_hdr, server_url->file ); @@ -254,10 +258,13 @@ http_send_request( URL_t *url, off_t pos ) { if (network_cookies_enabled) cookies_set( http_hdr, server_url->hostname, server_url->url ); http_set_field( http_hdr, "Connection: close"); - if (proxy) + if (proxy) { http_add_basic_proxy_authentication( http_hdr, url->username, url->password ); - else + if (server_url->username && server_url->password) + http_add_basic_authentication( http_hdr, server_url->username, server_url->password ); + } else { http_add_basic_authentication( http_hdr, url->username, url->password ); + } if( http_build_request( http_hdr )==NULL ) { goto err_out; } -- 1.7.3.1
On Mon, Oct 18, 2010 at 12:05:53PM +0200, Clément Bœsch wrote:
Currently, http_proxy:// does not forward server auth correcly, here is a fix. --- stream/network.c | 13 ++++++++++--- 1 files changed, 10 insertions(+), 3 deletions(-)
diff --git a/stream/network.c b/stream/network.c index 9f81609..1ec1220 100644 --- a/stream/network.c +++ b/stream/network.c @@ -209,7 +209,11 @@ http_send_request( URL_t *url, off_t pos ) { mp_msg(MSGT_NETWORK, MSGL_ERR, "Invalid URL '%s' to proxify\n", url->file+1); goto err_out; } - http_set_uri( http_hdr, server_url->url ); + snprintf(str, 256, "http://%s:%d%s", + server_url->hostname, + server_url->port ? server_url->port : 80, + server_url->file); + http_set_uri( http_hdr, str );
What is going wrong here, what is in server_url->url?
@@ -254,10 +258,13 @@ http_send_request( URL_t *url, off_t pos ) { if (network_cookies_enabled) cookies_set( http_hdr, server_url->hostname, server_url->url );
http_set_field( http_hdr, "Connection: close"); - if (proxy) + if (proxy) { http_add_basic_proxy_authentication( http_hdr, url->username, url->password ); - else + if (server_url->username && server_url->password) + http_add_basic_authentication( http_hdr, server_url->username, server_url->password ); + } else { http_add_basic_authentication( http_hdr, url->username, url->password ); + }
There's no need to add {} for the else part.
On Sun, Oct 24, 2010 at 08:38:43PM +0200, Reimar Döffinger wrote:
On Mon, Oct 18, 2010 at 12:05:53PM +0200, Clément Bœsch wrote:
Currently, http_proxy:// does not forward server auth correcly, here is a fix. --- stream/network.c | 13 ++++++++++--- 1 files changed, 10 insertions(+), 3 deletions(-)
diff --git a/stream/network.c b/stream/network.c index 9f81609..1ec1220 100644 --- a/stream/network.c +++ b/stream/network.c @@ -209,7 +209,11 @@ http_send_request( URL_t *url, off_t pos ) { mp_msg(MSGT_NETWORK, MSGL_ERR, "Invalid URL '%s' to proxify\n", url->file+1); goto err_out; } - http_set_uri( http_hdr, server_url->url ); + snprintf(str, 256, "http://%s:%d%s", + server_url->hostname, + server_url->port ? server_url->port : 80, + server_url->file); + http_set_uri( http_hdr, str );
What is going wrong here, what is in server_url->url?
http://login:password@host:8000/foo.ogg instead of http://host:8000/foo.ogg Authentication must be passed through Authentication header. This explains the diff below.
@@ -254,10 +258,13 @@ http_send_request( URL_t *url, off_t pos ) { if (network_cookies_enabled) cookies_set( http_hdr, server_url->hostname, server_url->url );
http_set_field( http_hdr, "Connection: close"); - if (proxy) + if (proxy) { http_add_basic_proxy_authentication( http_hdr, url->username, url->password ); - else + if (server_url->username && server_url->password) + http_add_basic_authentication( http_hdr, server_url->username, server_url->password ); + } else { http_add_basic_authentication( http_hdr, url->username, url->password ); + }
There's no need to add {} for the else part. _______________________________________________ MPlayer-dev-eng mailing list MPlayer-dev-eng@mplayerhq.hu https://lists.mplayerhq.hu/mailman/listinfo/mplayer-dev-eng
-- Clément B. Not sent from a jesusPhone.
On Sun, Oct 24, 2010 at 09:44:56PM +0200, Clément Bœsch wrote:
On Sun, Oct 24, 2010 at 08:38:43PM +0200, Reimar Döffinger wrote:
On Mon, Oct 18, 2010 at 12:05:53PM +0200, Clément Bœsch wrote:
Currently, http_proxy:// does not forward server auth correcly, here is a fix. --- stream/network.c | 13 ++++++++++--- 1 files changed, 10 insertions(+), 3 deletions(-)
diff --git a/stream/network.c b/stream/network.c index 9f81609..1ec1220 100644 --- a/stream/network.c +++ b/stream/network.c @@ -209,7 +209,11 @@ http_send_request( URL_t *url, off_t pos ) { mp_msg(MSGT_NETWORK, MSGL_ERR, "Invalid URL '%s' to proxify\n", url->file+1); goto err_out; } - http_set_uri( http_hdr, server_url->url ); + snprintf(str, 256, "http://%s:%d%s", + server_url->hostname, + server_url->port ? server_url->port : 80, + server_url->file); + http_set_uri( http_hdr, str );
What is going wrong here, what is in server_url->url?
http://login:password@host:8000/foo.ogg instead of http://host:8000/foo.ogg
At least I think it should use server_url->protocol instead of a fixed "http".
@@ -254,10 +258,13 @@ http_send_request( URL_t *url, off_t pos ) { if (network_cookies_enabled) cookies_set( http_hdr, server_url->hostname, server_url->url );
http_set_field( http_hdr, "Connection: close"); - if (proxy) + if (proxy) { http_add_basic_proxy_authentication( http_hdr, url->username, url->password ); - else + if (server_url->username && server_url->password) + http_add_basic_authentication( http_hdr, server_url->username, server_url->password ); + } else { http_add_basic_authentication( http_hdr, url->username, url->password ); + }
In the !proxy case server_url == url, and http_add_basic_authentication works with NULL password and is a NOP with NULL username, thus this whole code should be if (proxy) http_add_basic_proxy_authentication(http_hdr, url->username, url->password); http_add_basic_authentication(http_hdr, server_url->username, server_url->password);
On Sat, Nov 06, 2010 at 11:06:44AM +0100, Reimar Döffinger wrote:
On Sun, Oct 24, 2010 at 09:44:56PM +0200, Clément Bœsch wrote:
On Sun, Oct 24, 2010 at 08:38:43PM +0200, Reimar Döffinger wrote:
On Mon, Oct 18, 2010 at 12:05:53PM +0200, Clément Bœsch wrote:
Currently, http_proxy:// does not forward server auth correcly, here is a fix. --- stream/network.c | 13 ++++++++++--- 1 files changed, 10 insertions(+), 3 deletions(-)
diff --git a/stream/network.c b/stream/network.c index 9f81609..1ec1220 100644 --- a/stream/network.c +++ b/stream/network.c @@ -209,7 +209,11 @@ http_send_request( URL_t *url, off_t pos ) { mp_msg(MSGT_NETWORK, MSGL_ERR, "Invalid URL '%s' to proxify\n", url->file+1); goto err_out; } - http_set_uri( http_hdr, server_url->url ); + snprintf(str, 256, "http://%s:%d%s", + server_url->hostname, + server_url->port ? server_url->port : 80, + server_url->file); + http_set_uri( http_hdr, str );
What is going wrong here, what is in server_url->url?
http://login:password@host:8000/foo.ogg instead of http://host:8000/foo.ogg
At least I think it should use server_url->protocol instead of a fixed "http".
This patch is necessary with proxies like polipo. Requesting http_proxy://localhost:8123/http://login:password@host:8000/foo/bar.ogg, proxy receives: before the patch: GET http://login:password@host:8000/foo/bar.ogg HTTP/1.0 [...] Authorization: Basic bG9naW46cGFzc3dvcmQ= after the patch: GET http://host:8000/foo/bar.ogg HTTP/1.0 [...] Authorization: Basic bG9naW46cGFzc3dvcmQ= This allows the proxy to correctly forward the request (and not try to resolve login:password as URL). OK to apply? -- Clément B. Not sent from a jesusPhone.
On Thu, Nov 18, 2010 at 09:34:24PM +0100, Clément Bœsch wrote:
diff --git a/stream/network.c b/stream/network.c index 02ddcc8..6b7117c 100644 --- a/stream/network.c +++ b/stream/network.c @@ -209,7 +209,12 @@ http_send_request( URL_t *url, off_t pos ) { mp_msg(MSGT_NETWORK, MSGL_ERR, "Invalid URL '%s' to proxify\n", url->file+1); goto err_out; } - http_set_uri( http_hdr, server_url->url ); + snprintf(str, sizeof(str), "%s://%s:%d%s", + server_url->protocol, + server_url->hostname, + server_url->port ? server_url->port : 80, + server_url->file); + http_set_uri(http_hdr, str);
What's your opinion on removing user/password from ->url in url_new instead? I think it is quite ugly, but I guess I am fine with it if you think it's the best solution. I do feel a bit uncomfortable about using fixed-size, on-stack destination, URLs can get quite gigantic...
On Thu, Nov 18, 2010 at 10:32:50PM +0100, Reimar Döffinger wrote:
On Thu, Nov 18, 2010 at 09:34:24PM +0100, Clément Bœsch wrote:
diff --git a/stream/network.c b/stream/network.c index 02ddcc8..6b7117c 100644 --- a/stream/network.c +++ b/stream/network.c @@ -209,7 +209,12 @@ http_send_request( URL_t *url, off_t pos ) { mp_msg(MSGT_NETWORK, MSGL_ERR, "Invalid URL '%s' to proxify\n", url->file+1); goto err_out; } - http_set_uri( http_hdr, server_url->url ); + snprintf(str, sizeof(str), "%s://%s:%d%s", + server_url->protocol, + server_url->hostname, + server_url->port ? server_url->port : 80, + server_url->file); + http_set_uri(http_hdr, str);
What's your opinion on removing user/password from ->url in url_new instead? I think it is quite ugly, but I guess I am fine with it if you think it's the best solution. I do feel a bit uncomfortable about using fixed-size, on-stack destination, URLs can get quite gigantic...
Indeed, fixed-size for URLs is a problem. But I'm not sure about some issues that can be caused by the modification of the url field. The first one that come to my mind is the http_proxy url build in stream/network.c:178 for example. This will need modification, and it may be not the only case. However, it's still possible to add a field say "noauth_url". This can also be used when writing the stream URL on stdout (and avoid displaying the authentication). This solution looks like harmless. Patch attached with this solution.
_______________________________________________ MPlayer-dev-eng mailing list MPlayer-dev-eng@mplayerhq.hu https://lists.mplayerhq.hu/mailman/listinfo/mplayer-dev-eng
-- Clément B. Not sent from a jesusPhone.
On Fri, Nov 19, 2010 at 12:11:58AM +0100, Clément Bœsch wrote:
On Thu, Nov 18, 2010 at 10:32:50PM +0100, Reimar Döffinger wrote:
On Thu, Nov 18, 2010 at 09:34:24PM +0100, Clément Bœsch wrote:
diff --git a/stream/network.c b/stream/network.c index 02ddcc8..6b7117c 100644 --- a/stream/network.c +++ b/stream/network.c @@ -209,7 +209,12 @@ http_send_request( URL_t *url, off_t pos ) { mp_msg(MSGT_NETWORK, MSGL_ERR, "Invalid URL '%s' to proxify\n", url->file+1); goto err_out; } - http_set_uri( http_hdr, server_url->url ); + snprintf(str, sizeof(str), "%s://%s:%d%s", + server_url->protocol, + server_url->hostname, + server_url->port ? server_url->port : 80, + server_url->file); + http_set_uri(http_hdr, str);
What's your opinion on removing user/password from ->url in url_new instead? I think it is quite ugly, but I guess I am fine with it if you think it's the best solution. I do feel a bit uncomfortable about using fixed-size, on-stack destination, URLs can get quite gigantic...
Indeed, fixed-size for URLs is a problem. But I'm not sure about some issues that can be caused by the modification of the url field. The first one that come to my mind is the http_proxy url build in stream/network.c:178 for example. This will need modification, and it may be not the only case.
However, it's still possible to add a field say "noauth_url". This can also be used when writing the stream URL on stdout (and avoid displaying the authentication). This solution looks like harmless.
Patch attached with this solution.
Seems ok if you can do a good solution with that. Though could you use snprintf, just to be paranoid? Actually, I think there is an actual bug, the string there will become longer if url was a://b/c it will become a://b:80/c. Also :80 is wrong for e.g. ftp.
On Fri, Nov 19, 2010 at 07:54:06AM +0100, Reimar Döffinger wrote:
On Fri, Nov 19, 2010 at 12:11:58AM +0100, Clément Bœsch wrote:
On Thu, Nov 18, 2010 at 10:32:50PM +0100, Reimar Döffinger wrote:
On Thu, Nov 18, 2010 at 09:34:24PM +0100, Clément Bœsch wrote:
diff --git a/stream/network.c b/stream/network.c index 02ddcc8..6b7117c 100644 --- a/stream/network.c +++ b/stream/network.c @@ -209,7 +209,12 @@ http_send_request( URL_t *url, off_t pos ) { mp_msg(MSGT_NETWORK, MSGL_ERR, "Invalid URL '%s' to proxify\n", url->file+1); goto err_out; } - http_set_uri( http_hdr, server_url->url ); + snprintf(str, sizeof(str), "%s://%s:%d%s", + server_url->protocol, + server_url->hostname, + server_url->port ? server_url->port : 80, + server_url->file); + http_set_uri(http_hdr, str);
What's your opinion on removing user/password from ->url in url_new instead? I think it is quite ugly, but I guess I am fine with it if you think it's the best solution. I do feel a bit uncomfortable about using fixed-size, on-stack destination, URLs can get quite gigantic...
Indeed, fixed-size for URLs is a problem. But I'm not sure about some issues that can be caused by the modification of the url field. The first one that come to my mind is the http_proxy url build in stream/network.c:178 for example. This will need modification, and it may be not the only case.
However, it's still possible to add a field say "noauth_url". This can also be used when writing the stream URL on stdout (and avoid displaying the authentication). This solution looks like harmless.
Patch attached with this solution.
Seems ok if you can do a good solution with that. Though could you use snprintf, just to be paranoid?
Yep sure, patch updated.
Actually, I think there is an actual bug, the string there will become longer if url was a://b/c it will become a://b:80/c.
Yes indeed, this is fixed.
Also :80 is wrong for e.g. ftp.
It's not present anymore: port is not specified if not set now, just like previous patch. I also added the missing free, so it should be ok. -- Clément B. Not sent from a jesusPhone.
On Fri, Nov 19, 2010 at 02:57:28PM +0100, Clément Bœsch wrote:
On Fri, Nov 19, 2010 at 07:54:06AM +0100, Reimar Döffinger wrote:
On Fri, Nov 19, 2010 at 12:11:58AM +0100, Clément Bœsch wrote:
On Thu, Nov 18, 2010 at 10:32:50PM +0100, Reimar Döffinger wrote:
On Thu, Nov 18, 2010 at 09:34:24PM +0100, Clément Bœsch wrote:
diff --git a/stream/network.c b/stream/network.c index 02ddcc8..6b7117c 100644 --- a/stream/network.c +++ b/stream/network.c @@ -209,7 +209,12 @@ http_send_request( URL_t *url, off_t pos ) { mp_msg(MSGT_NETWORK, MSGL_ERR, "Invalid URL '%s' to proxify\n", url->file+1); goto err_out; } - http_set_uri( http_hdr, server_url->url ); + snprintf(str, sizeof(str), "%s://%s:%d%s", + server_url->protocol, + server_url->hostname, + server_url->port ? server_url->port : 80, + server_url->file); + http_set_uri(http_hdr, str);
What's your opinion on removing user/password from ->url in url_new instead? I think it is quite ugly, but I guess I am fine with it if you think it's the best solution. I do feel a bit uncomfortable about using fixed-size, on-stack destination, URLs can get quite gigantic...
Indeed, fixed-size for URLs is a problem. But I'm not sure about some issues that can be caused by the modification of the url field. The first one that come to my mind is the http_proxy url build in stream/network.c:178 for example. This will need modification, and it may be not the only case.
However, it's still possible to add a field say "noauth_url". This can also be used when writing the stream URL on stdout (and avoid displaying the authentication). This solution looks like harmless.
Patch attached with this solution.
Seems ok if you can do a good solution with that. Though could you use snprintf, just to be paranoid?
Yep sure, patch updated.
Actually, I think there is an actual bug, the string there will become longer if url was a://b/c it will become a://b:80/c.
Yes indeed, this is fixed.
Also :80 is wrong for e.g. ftp.
It's not present anymore: port is not specified if not set now, just like previous patch.
I also added the missing free, so it should be ok.
OK to apply? It's the last feature to make proxy authentication possible. -- Clément B. Not sent from a jesusPhone.
On Fri, Nov 19, 2010 at 02:57:28PM +0100, Clément Bœsch wrote:
+ noauth_len = strlen(Curl->protocol) + strlen(Curl->hostname) + + strlen(Curl->file) + sizeof("://:65535");
Eeek. Is there even a check that limits the port number to 65535? Either way I think this needs to be done in some simpler way. Maybe using the snprintf return value + 1 when calling it with size == 0. A bit of a waste of CPU time but shouldn't be that bad.
+ if (Curl->port) + snprintf(Curl->noauth_url, noauth_len, "%s://%s:%d%s", + Curl->protocol, Curl->hostname, Curl->port, Curl->file); + else + snprintf(Curl->noauth_url, noauth_len, "%s://%s%s", + Curl->protocol, Curl->hostname, Curl->file);
To avoid code duplication I'd put that maybe in a helper funtion that takes the noauth_len as argument.
On Sat, Nov 20, 2010 at 09:54:38PM +0100, Reimar Döffinger wrote:
On Fri, Nov 19, 2010 at 02:57:28PM +0100, Clément Bœsch wrote:
+ noauth_len = strlen(Curl->protocol) + strlen(Curl->hostname) + + strlen(Curl->file) + sizeof("://:65535");
Eeek. Is there even a check that limits the port number to 65535? Either way I think this needs to be done in some simpler way. Maybe using the snprintf return value + 1 when calling it with size == 0. A bit of a waste of CPU time but shouldn't be that bad.
+ if (Curl->port) + snprintf(Curl->noauth_url, noauth_len, "%s://%s:%d%s", + Curl->protocol, Curl->hostname, Curl->port, Curl->file); + else + snprintf(Curl->noauth_url, noauth_len, "%s://%s%s", + Curl->protocol, Curl->hostname, Curl->file);
To avoid code duplication I'd put that maybe in a helper funtion that takes the noauth_len as argument.
What about this? It may be a bit overkill thought… -- Clément B. Not sent from a jesusPhone.
On Sun, Nov 21, 2010 at 02:24:26PM +0100, Clément Bœsch wrote:
diff --git a/stream/url.c b/stream/url.c index 0469de9..90f41f7 100644 --- a/stream/url.c +++ b/stream/url.c @@ -58,6 +58,33 @@ URL_t *url_redirect(URL_t **url, const char *redir) { return res; }
+#ifdef __GNUC__ +static void make_noauth_url(URL_t *url, const char *fmt, ...) __attribute__ ((format(printf, 2, 3))); +#endif + +static void make_noauth_url(URL_t *url, const char *fmt, ...) +{ + va_list va1, va2; + int len; + + va_start(va1, fmt); + va_copy(va2, va1); + len = vsnprintf(NULL, 0, fmt, va1); + if (len < 0) { + url->noauth_url = NULL; + goto end; + } + len++; + url->noauth_url = malloc(len); + if (!url->noauth_url) + goto end; + vsnprintf(url->noauth_url, len, fmt, va2); + +end: + va_end(va1); + va_end(va2); +} + URL_t* url_new(const char* url) { int pos1, pos2,v6addr = 0; @@ -232,6 +259,17 @@ url_new(const char* url) { strcpy(Curl->file, "/"); }
+ if (Curl->port) + make_noauth_url(Curl, "%s://%s:%d%s", Curl->protocol, + Curl->hostname, Curl->port, Curl->file); + else + make_noauth_url(Curl, "%s://%s%s", Curl->protocol, + Curl->hostname, Curl->file); + if (Curl->noauth_url == NULL) { + mp_msg(MSGT_NETWORK, MSGL_FATAL, MSGTR_MemAllocFailed); + goto err_out; + }
This seems like a really bad way to split it. static int make_noauth_url(URL_t *url, char *dst, int dst_size) { if (url->port) return snprintf(dst, dst_size, "%s://%s:%d%s", url->protocol, url->hostname, url->port, url->file); else return snprintf(dst, dst_size, "%s://%s%s", url->protocol, url->hostname, url->file); } len = make_noauth_url(Curl, NULL, 0); if (len > 0) { Curl->noauth_url = malloc(len); if (!Curl->noauth_url) { ... } make_noauth_url(Curl, Curl->noauth_url, len); } Is far simpler. Or if you want to go with your approach it should be made generic in the form of a kind of "sprintf_alloc", without the URL_t argument but instead e.g. a char * return value - but I think that is overkill, and difficult to review (and I am still not a fan of the va_* stuff even though I guess that nowadays you can expect it to work correctly).
On Sun, Nov 21, 2010 at 02:35:03PM +0100, Reimar Döffinger wrote:
On Sun, Nov 21, 2010 at 02:24:26PM +0100, Clément Bœsch wrote:
diff --git a/stream/url.c b/stream/url.c index 0469de9..90f41f7 100644 --- a/stream/url.c +++ b/stream/url.c @@ -58,6 +58,33 @@ URL_t *url_redirect(URL_t **url, const char *redir) { return res; }
+#ifdef __GNUC__ +static void make_noauth_url(URL_t *url, const char *fmt, ...) __attribute__ ((format(printf, 2, 3))); +#endif + +static void make_noauth_url(URL_t *url, const char *fmt, ...) +{ + va_list va1, va2; + int len; + + va_start(va1, fmt); + va_copy(va2, va1); + len = vsnprintf(NULL, 0, fmt, va1); + if (len < 0) { + url->noauth_url = NULL; + goto end; + } + len++; + url->noauth_url = malloc(len); + if (!url->noauth_url) + goto end; + vsnprintf(url->noauth_url, len, fmt, va2); + +end: + va_end(va1); + va_end(va2); +} + URL_t* url_new(const char* url) { int pos1, pos2,v6addr = 0; @@ -232,6 +259,17 @@ url_new(const char* url) { strcpy(Curl->file, "/"); }
+ if (Curl->port) + make_noauth_url(Curl, "%s://%s:%d%s", Curl->protocol, + Curl->hostname, Curl->port, Curl->file); + else + make_noauth_url(Curl, "%s://%s%s", Curl->protocol, + Curl->hostname, Curl->file); + if (Curl->noauth_url == NULL) { + mp_msg(MSGT_NETWORK, MSGL_FATAL, MSGTR_MemAllocFailed); + goto err_out; + }
This seems like a really bad way to split it.
static int make_noauth_url(URL_t *url, char *dst, int dst_size) { if (url->port) return snprintf(dst, dst_size, "%s://%s:%d%s", url->protocol, url->hostname, url->port, url->file); else return snprintf(dst, dst_size, "%s://%s%s", url->protocol, url->hostname, url->file); }
len = make_noauth_url(Curl, NULL, 0); if (len > 0) { Curl->noauth_url = malloc(len); if (!Curl->noauth_url) { ... } make_noauth_url(Curl, Curl->noauth_url, len); }
Is far simpler. Or if you want to go with your approach it should be made generic in the form of a kind of "sprintf_alloc", without the URL_t argument but instead e.g. a char * return value - but I think that is overkill, and difficult to review (and I am still not a fan of the va_* stuff even though I guess that nowadays you can expect it to work correctly).
OK I see, then I'll commit this. Do you mind a reindent of stream/{url,network,http}.c after this commit? -- Clément B. Not sent from a jesusPhone.
On Sun, Nov 21, 2010 at 03:13:32PM +0100, Clément Bœsch wrote:
On Sun, Nov 21, 2010 at 02:35:03PM +0100, Reimar Döffinger wrote:
Is far simpler. Or if you want to go with your approach it should be made generic in the form of a kind of "sprintf_alloc", without the URL_t argument but instead e.g. a char * return value - but I think that is overkill, and difficult to review (and I am still not a fan of the va_* stuff even though I guess that nowadays you can expect it to work correctly).
OK I see, then I'll commit this. Do you mind a reindent of stream/{url,network,http}.c after this commit?
No, but I would vastly prefer if you only do a reindent, no adding line-breaks or spaces elsewhere or anything else that would show up in a diff -uwb and it uses 4-space indentation.
On Sun, Nov 21, 2010 at 04:37:34PM +0100, Reimar Döffinger wrote:
On Sun, Nov 21, 2010 at 03:13:32PM +0100, Clément Bœsch wrote:
On Sun, Nov 21, 2010 at 02:35:03PM +0100, Reimar Döffinger wrote:
Is far simpler. Or if you want to go with your approach it should be made generic in the form of a kind of "sprintf_alloc", without the URL_t argument but instead e.g. a char * return value - but I think that is overkill, and difficult to review (and I am still not a fan of the va_* stuff even though I guess that nowadays you can expect it to work correctly).
OK I see, then I'll commit this. Do you mind a reindent of stream/{url,network,http}.c after this commit?
No, but I would vastly prefer if you only do a reindent, no adding line-breaks or spaces elsewhere or anything else that would show up in a diff -uwb and it uses 4-space indentation.
Because of pending patches? Another reason? -- Clément B. Not sent from a jesusPhone.
On Sun, Nov 21, 2010 at 04:41:21PM +0100, Clément Bœsch wrote:
On Sun, Nov 21, 2010 at 04:37:34PM +0100, Reimar Döffinger wrote:
On Sun, Nov 21, 2010 at 03:13:32PM +0100, Clément Bœsch wrote:
On Sun, Nov 21, 2010 at 02:35:03PM +0100, Reimar Döffinger wrote:
Is far simpler. Or if you want to go with your approach it should be made generic in the form of a kind of "sprintf_alloc", without the URL_t argument but instead e.g. a char * return value - but I think that is overkill, and difficult to review (and I am still not a fan of the va_* stuff even though I guess that nowadays you can expect it to work correctly).
OK I see, then I'll commit this. Do you mind a reindent of stream/{url,network,http}.c after this commit?
No, but I would vastly prefer if you only do a reindent, no adding line-breaks or spaces elsewhere or anything else that would show up in a diff -uwb and it uses 4-space indentation.
Because of pending patches? Another reason?
It's not much effort for me or anyone else to verify you did not make a stupid mistake and broke some obscure feature in the process. There are other solutions (e.g. verifying the binary output is the same), and Diego convinced me to accept that for reindenting major parts of the code, but for a one-off I consider it too much effort.
On Sun, Nov 21, 2010 at 03:13:32PM +0100, Clément Bœsch wrote:
From 36aa3e6091e8ca8b532ce5607aa3dbc2167844fb Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Cl=C3=A9ment=20B=C5=93sch?= <ubitux@gmail.com> Date: Thu, 18 Nov 2010 21:27:03 +0100 Subject: [PATCH] Do not keep authentication in URL on connection through a proxy
Applied. -- Clément B. Not sent from a jesusPhone.
Add a http_proxy example in the manpage because it's laking one and the syntax is not really explicit. --- DOCS/man/en/mplayer.1 | 6 ++++++ 1 files changed, 6 insertions(+), 0 deletions(-) diff --git a/DOCS/man/en/mplayer.1 b/DOCS/man/en/mplayer.1 index 5610cd6..007b7e5 100644 --- a/DOCS/man/en/mplayer.1 +++ b/DOCS/man/en/mplayer.1 @@ -11666,6 +11666,12 @@ mplayer http://mplayer.hq/example.avi .fi . .PP +.B Stream from HTTP (with auth) through a HTTP proxy (with auth): +.nf +mplayer http_proxy://proxy_user:proxy_passwd@proxy:3128/http://user:passwd@mplayer.hq/example.avi +.fi +. +.PP .B Stream using RTSP: .nf mplayer rtsp://server.example.com/streamName -- 1.7.3.1
On Mon, Oct 18, 2010 at 12:05:54PM +0200, Clément Bœsch wrote:
Add a http_proxy example in the manpage because it's laking one and the syntax is not really explicit.
OK to apply this small example, or a more complete documentation would be appreciated? Maybe more examples with '%xx' stuff?
--- DOCS/man/en/mplayer.1 | 6 ++++++ 1 files changed, 6 insertions(+), 0 deletions(-)
diff --git a/DOCS/man/en/mplayer.1 b/DOCS/man/en/mplayer.1 index 5610cd6..007b7e5 100644 --- a/DOCS/man/en/mplayer.1 +++ b/DOCS/man/en/mplayer.1 @@ -11666,6 +11666,12 @@ mplayer http://mplayer.hq/example.avi .fi . .PP +.B Stream from HTTP (with auth) through a HTTP proxy (with auth): +.nf +mplayer http_proxy://proxy_user:proxy_passwd@proxy:3128/http://user:passwd@mplayer.hq/example.avi +.fi +. +.PP .B Stream using RTSP: .nf mplayer rtsp://server.example.com/streamName -- 1.7.3.1
-- Clément B. Not sent from a jesusPhone.
Subject should be self explanatory. --- stream/http.c | 32 ++++++++++++++++---------------- 1 files changed, 16 insertions(+), 16 deletions(-) diff --git a/stream/http.c b/stream/http.c index c5f1b12..67ce1e8 100644 --- a/stream/http.c +++ b/stream/http.c @@ -335,7 +335,7 @@ http_response_append( HTTP_header_t *http_hdr, char *response, int length ) { } http_hdr->buffer = realloc( http_hdr->buffer, http_hdr->buffer_size+length+1 ); if( http_hdr->buffer==NULL ) { - mp_msg(MSGT_NETWORK,MSGL_FATAL,"Memory (re)allocation failed\n"); + mp_msg(MSGT_NETWORK,MSGL_FATAL,MSGTR_MemAllocFailed); return -1; } memcpy( http_hdr->buffer+http_hdr->buffer_size, response, length ); @@ -372,7 +372,7 @@ http_response_parse( HTTP_header_t *http_hdr ) { len = hdr_ptr-http_hdr->buffer; http_hdr->protocol = malloc(len+1); if( http_hdr->protocol==NULL ) { - mp_msg(MSGT_NETWORK,MSGL_FATAL,"Memory allocation failed\n"); + mp_msg(MSGT_NETWORK,MSGL_FATAL,MSGTR_MemAllocFailed); return -1; } strncpy( http_hdr->protocol, http_hdr->buffer, len ); @@ -400,7 +400,7 @@ http_response_parse( HTTP_header_t *http_hdr ) { len = ptr-hdr_ptr; http_hdr->reason_phrase = malloc(len+1); if( http_hdr->reason_phrase==NULL ) { - mp_msg(MSGT_NETWORK,MSGL_FATAL,"Memory allocation failed\n"); + mp_msg(MSGT_NETWORK,MSGL_FATAL,MSGTR_MemAllocFailed); return -1; } strncpy( http_hdr->reason_phrase, hdr_ptr, len ); @@ -431,7 +431,7 @@ http_response_parse( HTTP_header_t *http_hdr ) { if( len==0 ) break; field = realloc(field, len+1); if( field==NULL ) { - mp_msg(MSGT_NETWORK,MSGL_ERR,"Memory allocation failed\n"); + mp_msg(MSGT_NETWORK,MSGL_ERR,MSGTR_MemAllocFailed); return -1; } strncpy( field, hdr_ptr, len ); @@ -464,7 +464,7 @@ http_build_request( HTTP_header_t *http_hdr ) { else { uri = malloc(strlen(http_hdr->uri) + 1); if( uri==NULL ) { - mp_msg(MSGT_NETWORK,MSGL_ERR,"Memory allocation failed\n"); + mp_msg(MSGT_NETWORK,MSGL_ERR,MSGTR_MemAllocFailed); return NULL; } strcpy(uri,http_hdr->uri); @@ -492,7 +492,7 @@ http_build_request( HTTP_header_t *http_hdr ) { } http_hdr->buffer = malloc(len+1); if( http_hdr->buffer==NULL ) { - mp_msg(MSGT_NETWORK,MSGL_ERR,"Memory allocation failed\n"); + mp_msg(MSGT_NETWORK,MSGL_ERR,MSGTR_MemAllocFailed); return NULL; } http_hdr->buffer_size = len; @@ -523,7 +523,7 @@ http_get_field( HTTP_header_t *http_hdr, const char *field_name ) { http_hdr->field_search_pos = http_hdr->first_field; http_hdr->field_search = realloc( http_hdr->field_search, strlen(field_name)+1 ); if( http_hdr->field_search==NULL ) { - mp_msg(MSGT_NETWORK,MSGL_FATAL,"Memory allocation failed\n"); + mp_msg(MSGT_NETWORK,MSGL_FATAL,MSGTR_MemAllocFailed); return NULL; } strcpy( http_hdr->field_search, field_name ); @@ -558,13 +558,13 @@ http_set_field( HTTP_header_t *http_hdr, const char *field_name ) { new_field = malloc(sizeof(HTTP_field_t)); if( new_field==NULL ) { - mp_msg(MSGT_NETWORK,MSGL_FATAL,"Memory allocation failed\n"); + mp_msg(MSGT_NETWORK,MSGL_FATAL,MSGTR_MemAllocFailed); return; } new_field->next = NULL; new_field->field_name = malloc(strlen(field_name)+1); if( new_field->field_name==NULL ) { - mp_msg(MSGT_NETWORK,MSGL_FATAL,"Memory allocation failed\n"); + mp_msg(MSGT_NETWORK,MSGL_FATAL,MSGTR_MemAllocFailed); free(new_field); return; } @@ -585,7 +585,7 @@ http_set_method( HTTP_header_t *http_hdr, const char *method ) { http_hdr->method = malloc(strlen(method)+1); if( http_hdr->method==NULL ) { - mp_msg(MSGT_NETWORK,MSGL_FATAL,"Memory allocation failed\n"); + mp_msg(MSGT_NETWORK,MSGL_FATAL,MSGTR_MemAllocFailed); return; } strcpy( http_hdr->method, method ); @@ -597,7 +597,7 @@ http_set_uri( HTTP_header_t *http_hdr, const char *uri ) { http_hdr->uri = malloc(strlen(uri)+1); if( http_hdr->uri==NULL ) { - mp_msg(MSGT_NETWORK,MSGL_FATAL,"Memory allocation failed\n"); + mp_msg(MSGT_NETWORK,MSGL_FATAL,MSGTR_MemAllocFailed); return; } strcpy( http_hdr->uri, uri ); @@ -614,7 +614,7 @@ http_add_authentication( HTTP_header_t *http_hdr, const char *username, const ch username_decoded = malloc(strlen(username) + 1); if (username_decoded == NULL) { - mp_msg(MSGT_NETWORK, MSGL_FATAL, "Memory allocation failed\n"); + mp_msg(MSGT_NETWORK, MSGL_FATAL, MSGTR_MemAllocFailed); goto out; } url_unescape_string(username_decoded, username); @@ -623,7 +623,7 @@ http_add_authentication( HTTP_header_t *http_hdr, const char *username, const ch if( password!=NULL ) { password_decoded = malloc(strlen(password) + 1); if (password_decoded == NULL) { - mp_msg(MSGT_NETWORK, MSGL_FATAL, "Memory allocation failed\n"); + mp_msg(MSGT_NETWORK, MSGL_FATAL, MSGTR_MemAllocFailed); goto out; } url_unescape_string(password_decoded, password); @@ -633,7 +633,7 @@ http_add_authentication( HTTP_header_t *http_hdr, const char *username, const ch usr_pass = malloc(strlen(username)+pass_len+2); if( usr_pass==NULL ) { - mp_msg(MSGT_NETWORK,MSGL_FATAL,"Memory allocation failed\n"); + mp_msg(MSGT_NETWORK,MSGL_FATAL,MSGTR_MemAllocFailed); goto out; } @@ -643,7 +643,7 @@ http_add_authentication( HTTP_header_t *http_hdr, const char *username, const ch encoded_len = strlen(usr_pass)*2; b64_usr_pass = malloc(encoded_len); if( b64_usr_pass==NULL ) { - mp_msg(MSGT_NETWORK,MSGL_FATAL,"Memory allocation failed\n"); + mp_msg(MSGT_NETWORK,MSGL_FATAL,MSGTR_MemAllocFailed); goto out; } @@ -657,7 +657,7 @@ http_add_authentication( HTTP_header_t *http_hdr, const char *username, const ch auth = malloc(encoded_len + strlen(auth_str) + 1); if( auth==NULL ) { - mp_msg(MSGT_NETWORK,MSGL_FATAL,"Memory allocation failed\n"); + mp_msg(MSGT_NETWORK,MSGL_FATAL,MSGTR_MemAllocFailed); goto out; } -- 1.7.3.1
str has 256 Bytes but could be changed. --- stream/network.c | 12 ++++++------ 1 files changed, 6 insertions(+), 6 deletions(-) diff --git a/stream/network.c b/stream/network.c index 1ec1220..a571e06 100644 --- a/stream/network.c +++ b/stream/network.c @@ -209,7 +209,7 @@ http_send_request( URL_t *url, off_t pos ) { mp_msg(MSGT_NETWORK, MSGL_ERR, "Invalid URL '%s' to proxify\n", url->file+1); goto err_out; } - snprintf(str, 256, "http://%s:%d%s", + snprintf(str, sizeof(str), "http://%s:%d%s", server_url->hostname, server_url->port ? server_url->port : 80, server_url->file); @@ -219,14 +219,14 @@ http_send_request( URL_t *url, off_t pos ) { http_set_uri( http_hdr, server_url->file ); } if (server_url->port && server_url->port != 80) - snprintf(str, 256, "Host: %s:%d", server_url->hostname, server_url->port ); + snprintf(str, sizeof(str), "Host: %s:%d", server_url->hostname, server_url->port ); else - snprintf(str, 256, "Host: %s", server_url->hostname ); + snprintf(str, sizeof(str), "Host: %s", server_url->hostname ); http_set_field( http_hdr, str); if (network_useragent) - snprintf(str, 256, "User-Agent: %s", network_useragent); + snprintf(str, sizeof(str), "User-Agent: %s", network_useragent); else - snprintf(str, 256, "User-Agent: %s", mplayer_version); + snprintf(str, sizeof(str), "User-Agent: %s", mplayer_version); http_set_field(http_hdr, str); if (network_referrer) { @@ -251,7 +251,7 @@ http_send_request( URL_t *url, off_t pos ) { if(pos>0) { // Extend http_send_request with possibility to do partial content retrieval - snprintf(str, 256, "Range: bytes=%"PRId64"-", (int64_t)pos); + snprintf(str, sizeof(str), "Range: bytes=%"PRId64"-", (int64_t)pos); http_set_field(http_hdr, str); } -- 1.7.3.1
On Mon, Oct 18, 2010 at 12:05:56PM +0200, Clément Bœsch wrote:
str has 256 Bytes but could be changed.
This and the previous one seem obviously ok, though it would be better to first "fix" the code than adding new code the "bad" way and then fixing it.
The http_proxy://l:p@proxy/http://user:passwd@host/ now works, but not http_proxy://l:p@proxy/http://host/ -user user -passwd passwd, so the patch fixes this. --- stream/network.c | 2 ++ 1 files changed, 2 insertions(+), 0 deletions(-) diff --git a/stream/network.c b/stream/network.c index a571e06..d78df29 100644 --- a/stream/network.c +++ b/stream/network.c @@ -262,6 +262,8 @@ http_send_request( URL_t *url, off_t pos ) { http_add_basic_proxy_authentication( http_hdr, url->username, url->password ); if (server_url->username && server_url->password) http_add_basic_authentication( http_hdr, server_url->username, server_url->password ); + else if (network_username && network_password) + http_add_basic_authentication( http_hdr, network_username, network_password ); } else { http_add_basic_authentication( http_hdr, url->username, url->password ); } -- 1.7.3.1
participants (2)
-
Clément Bœsch -
Reimar Döffinger