60228 - Applet doing POST to an https connection over proxy doesn't work.

Status: VERIFIED FIXED

(Core Graveyard :: Java: OJI, defect, P3)

Description

[edburns]

Applet doing POST to an https connection over proxy doesn't work.

Comment 1

[edburns]

I'm finding that if an applet does this:

          URLConnection inConn = url.openConnection(); 
          inConn.setDoOutput(true);

which means POST to url, the POST data doesn't get successfully written IF (and 
only if) the url is an https url through proxy.

Comment 2

[edburns]

Attachment: tar.gz of testcase. Iteration one.

Comment 3

[edburns]

Reproducible: always
Steps to reproduce:

1. Build the RTM branch of mozilla

1.5 Install the java plugin

2. Install PSM 1.4 (don't ask me where or how to do this)

3. Untar attachment http://bugzilla.mozilla.org/showattachment.cgi?
attach_id=19285 to local disk or something.

4. Visit file:///<TESTCASE_UNTAR_PATH>/60228/html/Gap.html

Expected results: the applet should print out some text from the Gap site about 
an incorrect username and password

Actual results: nothing happens.

Comment 4

[edburns]

Here is the reason why I think it's broken:

Short version:

So, the moral of the story is: how do I properly set the upload stream
of an nsHTTPRequest instance when mDoingProxySSLConnect is true?

Long version:

The Java plugin calls nsPluginHostImpl::PostURL(), with the following
postData and postDataLength, respectively:

"
ContentLength: 39\r\n
Content-Type: application/x-www-form-urlencoded\r\n\r\n
eml_id=username
web_mem_psw=password
"

109

This immediately causes mozilla to ask for an nsIHTTPChannel and set the
upload stream to an nsIInputStream created from the above data:

nsHTTPRequest::SetUploadStream(nsIInputStream * 0x10af4824) line 1235
nsHTTPChannel::SetUploadStream(nsHTTPChannel * const 0x10af5940, nsIInputStream 
* 0x10af4824) line 2869
nsPluginHostImpl::NewPluginURLStream(nsPluginHostImpl * const 0x01a24ea0, const 
nsString & {...}, nsIPluginInstance * 0x0e9a9a68, nsIPluginStreamListener * 
0x0094e194, void * 0x00156690, unsigned int 109, const char * 0x00000000, 
unsigned int 0) line 3465
nsPluginHostImpl::PostURL(nsPluginHostImpl * const 0x01a24ea0, nsISupports * 
0x0e9a9a80, const char * 0x0094f8e0, unsigned int 109, const char * 0x00156690, 
int 0, const char * 0x00000000, nsIPluginStreamListener * 0x0094e194, const 
char * 0x00000000, const char * 0x00000000, int 0, unsigned int 0, const char * 
0x00000000) line 1713 + 46 bytes

Please see
<http://lxr.mozilla.org/mozilla/source/modules/plugin/nglsrc/nsPluginHostImpl.cp
p#3471>,
included here for convenience:

          if(aPostData)
            {
              nsCOMPtr<nsISupports> result = nsnull;
              nsCOMPtr<nsIInputStream> postDataStream = nsnull;
              if (aPostData) {
                NS_NewByteInputStream(getter_AddRefs(result),
                                      (const char *) aPostData, aPostDataLen);
                if (result) {
                  postDataStream = do_QueryInterface(result, &rv);
                }
              }
              
              // XXX it's a bit of a hack to rewind the postdata stream
              // here but it has to be done in case the post data is
              // being reused multiple times.
              
              nsCOMPtr<nsIRandomAccessStore> 
                postDataRandomAccess(do_QueryInterface(postDataStream));
              if (postDataRandomAccess)
                {
                  postDataRandomAccess->Seek(PR_SEEK_SET, 0);
                }
              
              nsCOMPtr<nsIAtom> method = NS_NewAtom ("POST");
              httpChannel->SetRequestMethod(method);
              httpChannel->SetUploadStream(postDataStream);
            }

This appears to be fine, the nsHTTPRequest's mInputStream ivar is set
properly.  However, later on, it is overwritten with nsnull, here:

nsHTTPRequest::GetUploadStream(nsIInputStream * * 0x10abb60c) line 1215
nsHTTPPipelinedRequest::WriteRequest(nsIInputStream * 0x00000000) line 650 + 35 
bytes
nsHTTPChannel::Open(int 0) line 1590 + 23 bytes
nsHTTPChannel::AsyncRead(nsHTTPChannel * const 0x10af5940, nsIStreamListener * 
0x10aa48c0, nsISupports * 0x00000000) line 339
nsPluginHostImpl::NewPluginURLStream(nsPluginHostImpl * const 0x01a24ea0, const 
nsString & {...}, nsIPluginInstance * 0x0e9a9a68, nsIPluginStreamListener * 
0x0094e194, void * 0x00156690, unsigned int 109, const char * 0x00000000, 
unsigned int 0) line 3475 + 47 bytes
nsPluginHostImpl::PostURL(nsPluginHostImpl * const 0x01a24ea0, nsISupports * 
0x0e9a9a80, const char * 0x0094f8e0, unsigned int 109, const char * 0x00156690, 
int 0, const char * 0x00000000, nsIPluginStreamListener * 0x0094e194, const 
char * 0x00000000, const char * 0x00000000, int 0, unsigned int 0, const char * 
0x00000000) line 1713 + 46 bytes

because ivar mDoingProxySSLConnect is PR_TRUE!:

Please see
<http://lxr.mozilla.org/mozilla/source/netwerk/protocol/http/src/nsHTTPRequest.c
pp#1220>
included here for convenience:

nsresult
nsHTTPRequest::GetUploadStream(nsIInputStream** o_UploadStream)
{
    if (!o_UploadStream)
        return NS_ERROR_NULL_POINTER;
    
    // until we are in process of SSL connect no upload stream avaible 
    if (mDoingProxySSLConnect) 
        *o_UploadStream = nsnull;
    else
        *o_UploadStream = mInputStream;
    NS_IF_ADDREF(*o_UploadStream);

    return NS_OK;

}

SO, the moral of the story is: how do I properly set the upload stream
when mDoingProxySSLConnect is true?

Thanks,

Ed

Comment 5

[edburns]

This is the "easy to test" version of bug 36212.  You don't have to be a sun 
employee with a token card to see this bug!

George, can you please add rich to cc?

Thanks,

Ed

Comment 6

[drapeau]

Looks like Rich is already CC'd on this bug.

Maybe gagan needs to be cc'd, too, in case he can help.

Comment 7

[edburns]

You also need to grant AllPermission in your java.policy file in order to see
this bug:

Comment 8

[edburns]

It turns out that the postData is correctly getting written to the https 
connection.  Therefore, I have no idea why this bug exists.

I have called Gagan Saksena and asked for help and have received no reply.

I am stuck.

Comment 9

[edburns]

Attachment: output of NSPR_LOG_MODULES=nsHTTPProtocol:3

Comment 10

[edburns]

This proves that the applet is able to successfully write data to the https 
server over a proxy.  Please look at "the attachment": 

http://bugzilla.mozilla.org/showattachment.cgi?attach_id=19418

for content.

On line 2071 of the attachment we see this:

=== Start
POST /http%3A//centralapp3.central.sun.com/cgi-bin/NetMail/cust_mailrc.cgi 
HTTP/1.1
Cookie: sid=33e066a1d0d30821
Host: central.sun.net
User-Agent: Mozilla/5.0 (X11; U; SunOS 5.7 sun4u; en-US; m18) Gecko/20001114
Accept: */*
Accept-Language: en
Accept-Encoding: gzip,deflate,compress,identity
Keep-Alive: 300
Connection: keep-alive
=== End


Then on line 2094, we see this:

1[62d78]: nsHTTPRequest [this=d36bf0]. Starting to write data to the server.
1[62d78]: nsHTTPRequest [this=d36bf0]. Writing PUT/POST data to the server.
1[62d78]: nsHTTPRequest [this=d36bf0]. Starting to write data to the server.
1[62d78]: nsHTTPRequest [this=d36bf0]. Finished writing request to server.     
Status: 0

Thus, the problem is NOT that https post fails.

To whom can I reassign this bug?

Ed

Comment 11

[edburns]

Please note that the testcase doesn't correctly represent the bug.  Can someone 
point me to an https url to which I can POST some random data?

Comment 12

[Nikolay Igotti]

I think I can help with this bug.  UploadStream for request is nsnull in 1st
phase of connecting to HTTPS site using proxy. It's part of my fix for HTTPS 
through proxy fix. To use upload stream you should get it _after_ HTTPS
connection complete. Ed, if you want I can try to fix this bug.

Comment 13

[drapeau]

Nikolay: please do fix this bug and post the diffs as an attachment to this bug.
 Then please ask for a review, and Rich can help you get it checked in if you
can do all of this before Ed returns from his vacation, next Tuesday (November 28).

Thank you very much!

Comment 14

[edburns]

Yes, please have at it Nikolay.  However, please note that according to the 
NSPR_LOG_MODULES output, the post data is correctly getting sent.  I don't 
think the post data is the problem.  However, I could be wrong.

Ed

Comment 15

[Xiaobin Lu]

Add myself to the CC list.

Comment 16

[edburns]

Attachment: cvs diff -u of possible fix for this bug, first iteration. Please test.

Comment 17

[edburns]

Could someone with a working PSM/Debug Mozilla/Java Plugin please test today's 
patch with the Sun.Net java mail case?  Thanks,

Ed

Comment 18

[Nikolay Igotti]

With https://dubhe1/60228/https.html it doesn't work. Moreover, I can't use 
applets anymore, after visiting this URL. I'll try to find out reasons of this
behavior.

Comment 19

[edburns]

I have verified that at least the browser correctly deals with request headers
being placed in the postData param to nsPluginHostImpl::PostURL().  Here is
the actual request received from the browser in the case of NO PROXY:

POST /test.html HTTP/1.1
Host: arago.eng.sun.com:8090
User-Agent: Mozilla/5.0 (Windows; U; WinNT4.0; en-US; m18) Gecko/20001117
Accept: */*
Accept-Language: en
Accept-Encoding: gzip,deflate,compress,identity
Keep-Alive: 300
Connection: keep-alive
Content-Length: 25
Content-Type: application/x-www-form-urlencoded

string=Nobody&data=none

Comment 20

[edburns]

This might be the reason for the problem.  I'm finding that when the applet 
does POST, there is no "\r\n\r\n" at the end of the HTTP request:  Here's the 
head and tail ends of the request sent by the test applet:

(gdb) print buf
$24 = 0x22800 "POST http://arago.eng.sun.com:8090/test.html HTTP/1.1\r\nHost: 
arago.eng.sun.com:8090\r\nUser-Agent: Mozilla/5.0 (Windows; U; WinNT4.0; en-US; 
m18) Gecko/20001117\r\nAccept: */*\r\nAccept-Language: en\r\nAccept"...

(gdb) print (buf + 260)
$27 = 0x22904 "Connection: keep-alive\r\nContent-Length: 25\r\nContent-Type: 
application/x-www-form-urlencoded\r\n\nstring=Nobody&data=none\r\n"

Note the "\r\n\n" between the last header and the post data.  Contrast this 
with the request from the browser just trying to visit a page:

(gdb) print buf
$29 = 0x22800 "GET http://www.yahoo.com/index.html HTTP/1.1\r\nCookie: 
B=dhfm33ct3fh6a&b=2\r\nHost: www.yahoo.com\r\nUser-Agent: Mozilla/5.0 (Windows; 
U; WinNT4.0; en-US; m18) Gecko/20001117\r\nAccept: */*\r\nAccept-Language:"...

(gdb) print (buf + 250)
$33 = 0x228fa "ty\r\nKeep-Alive: 300\r\nConnection: keep-alive\r\n\r\n"

Note this does have a "\r\n\r\n" after the last header.  This would cause the 
http server to not respond, since the HTTP request was never properly 
terminated.

Comment 21

[edburns]

It turns out that the nsHTTPRequest code EXPECTS the post input stream to 
include headers "\r\n\r\n" data.  This isn't always a valid assumption.  

Comment 22

[edburns]

It turns out this is indeed a bug in the plugin.  When the plugin calls 
nsPluginHostImpl::PostURL() it passes post data with headers and data in it.  
This is ok, except for the fact that the headers aren't separated from the data 
by the requisite "\r\n\r\n".  Since nsHTTPRequest doesn't check for this, it 
just assumes the postData will have "\r\n\r\n" the POST HTTP request sent to 
the server is malformed.  

Comment 23

[edburns]

Attachment: cvs diff -u of fix for this bug, second iteration

Comment 24

[edburns]

This fix accounts for the fact that the java plugin puts "\r\n\n"
between the headers and the post data, rather than the correct
"\r\n\r\n".  A more robust fix would be to fireproof this at the level
of nsHTTPRequest.cpp, however, at that point the postData is an opaque
nsIInputStream.

This fix adds a new method to nsPluginHostImpl, FixPostData():

+  /**
+
+   * called from NewPluginURLStream <P>
+
+   * This method takes postData and makes it correct according to the
+   * assumption of nsHTTPRequest.cpp that postData include "\r\n\r\n". <P>
+
+   * This method assumes inPostData DOES NOT already have "\r\n\r\n". <P>
+
+   * This method will search for "\r\n\n", which indicates the
+   * end of the last header.  It will then search for the first
+   * non-whitespace character after the last header.  It will then
+   * create a new buffer with the existing headers, a correct
+   * "\r\n\r\n", then the post data.  If no "\r\n" is found, the data
+   * does not contain headers, and a simple "\r\n\r\n" is prepended to
+   * the buffer.
+
+   * @param inPostData, the post data from NewPluginURLStream
+
+   * @param the length of inPostData
+
+   * @param outPostData the buffer which must be freed with delete [].
+
+   * @param outPostDataLen the length of outPostData
+  
+   */

* Right now this leaks!  I feel NS_NewByteInputStream
* should make a copy of the data, so I can safely delete
* it here.  As it stands now, NS_NewByteInputStream saves
* a pointer to the data, which isn't guaranteed to stick
* around.  As evidenced by the fact that if I delete this
* here, as I should, the result from
* NS_NewByteInputStream is bogus, since the rug has been
* pulled out from under it.

Comment 25

[Eric Pollmann]

A few comments:

1) The leak

You can use NS_NewPostDataStream instead of NS_NewByteInputStream, like this:

...
#include "nsNetUtil.h"
...
rv = NS_NewPostDataStream(&postDataStream, PR_FALSE, (const char *) aPostData, 0);
...

Or you could use the underlying implementation for NS_NewPostDataStream,
NS_NewCStringInputStream.  Either way, this implementation does make a copy of
the buffer on construction so you can delete your copy.

2) The other leak  ;)

+  if (!(newBuf = new char[inPostDataLen + 4])) {
+    return NS_ERROR_NULL_POINTER;
+  }
+  nsCRT::memset(newBuf, 0, inPostDataLen + 4);
+
+  if (!(crlf = PL_strstr(postData, "\r\n\n"))) {

Add a delete here: delete [] newBuf;

+    return NS_ERROR_NULL_POINTER;
+  }
+  headersLen = crlf - postData;

3) Overall, I wonder why this change is needed:  Is this a legacy plugin and Nav
4.x also corrects faulty headers like this?  Or is this a new plugin, and if so,
why can't it be fixed instead of compensating for it on the browser side?

Anyway, with the two leaks fixed, r=[e-mail]

Comment 26

[edburns]

Hi Eric and Chris,

Thanks for your feedback and suggestions.  I have incorporated your suggestions 
and am about to post the third iteration of this bug fix for your review and 
approval.

I have talked to Stanley Ho, the man behind the Sun Java Plugin, and he has 
told me they have no plans to fix this bug.  Rather, the java plugin in the 
future will not rely on the browser for HTTPS post services.  

This means that for Netscape 6.5, the Sun Java plugin (which is NOT a legacy 
plugin, mind you), will not be able to have applets that post data to a secure 
connection without this bugfix.

Please let me check it into the trunk.  It's very important to my customers.

The patch will do nothing if the post data already contains the 
required "\r\n\r\n".

Thanks,

Ed

Comment 27

[edburns]

Attachment: cvs diff -u of fix for this bug, third iteration

Comment 28

[Eric Pollmann]

Looks good to me - r=[e-mail]

Comment 29

[Chris Waterson]

Comments...

- What's with all the random whitespace in the doc comments?

- This probably needs to be removed...

@@ -1658,6 +1657,7 @@
                                                                                
PRUint32 postHeadersLength, 
                                                                                
const char* postHeaders)
 {
+  url = "http://arago.eng.sun.com:8090/test.html";
   nsAutoString      string; string.AssignWithConversion(url);
   nsIPluginInstance *instance;
   nsresult          rv;

- You never use `rv' in `FixPostData()'.

- Do you want to add NS_ASSERTION (or NS_ENSURE_ARG) at the head of 
`FixPostData()' to alert when evil error conditions arise?

- If `FixPostData()' fails, the caller just silently continues on its merry way. 
Should we assert?

Comment 30

[edburns]

Hi Chris,

What whitespace?  In nsPluginHostImpl.h?  That's just so M-Q in xemacs formats 
the paragraphs correctly.  I've removed the whitespace.

I've removed the test url.  (I would have done so before checking in of course).

I've removed the rv from FixPostData.

If FixPostData fails, yes we continue with the original implementation.  My 
goal for this fix was to be as non-invasive as possible.  I think there is some 
value in continuing even if FixPostData fails because someone could have 
implemented a fix in the future in another part of the code.  It's better to 
give them a crack at it.

Of course, if you insist, I can assert if FixPostData fails.

Please let me know if I have sr=waterson.

Thanks,

Ed

Comment 31

[drapeau]

waterson: thank you for the previous review.  We're trying to include this in an
internal deployment release at Sun (we'd like to reach dev-complete on Tuesday,
1/16) followed fairly soon by another external push.  The Solaris version of the
browser has a number of fixes that solve common crashers and deployment
problems.  This bug is one of the three or so remaining show-stoppers.  That's
the nature of the urgency on this one.


If you can look at Ed's revised patch soon and sr=, we would very much
appreciate it.

Comment 32

[Chris Waterson]

please attach a revised patch to the bug. thanks!

Comment 33

[edburns]

Chris, I can't attach a revised patch.  My hard drive crashed this morning and 
I won't be able to get a tree set up before I have to leave town and lend my 
machine to someone else.

George is going to take over getting this checked in.  My revised patch was 
simply the "third iteration" patch, with your changes regarding FixPostData, 
nsresult, the url var integrated into it. 

Whoever checks this in will have to re-do that integration.

Ed

Comment 34

[Chris Waterson]

Ok, george, when you get a patch, could you attach it here? thanks.

Comment 35

[drapeau]

Yep, will do.  Thanks again, Chris.

Comment 36

[Xiaobin Lu]

Attachment: Forth iteration of patch, modification made based on Chris's suggestion

Comment 37

[Xiaobin Lu]

Hi, Chris:
    I posted the new patch based on your suggestion. Would you like to give us 
a quick review? Thanks in advance for your approval!

Xiaobin

Comment 38

[Rich Burridge]

Chris: note that Xiaobin is standing in for Ed Burns this week. Ed is on
vacation. I'm standing in for George today to (who is also on vacation).
Xiaobin has attached a new diff with your suggested adjustments. Could you
give it your sr= blessing, and we'll then get it integrated into the trunk
and the OEM branch. Thanks.

Comment 39

[Xiaobin Lu]

Attachment: rv was removed

Comment 40

[Xiaobin Lu]

Chris:
   Please open the latest patch for reviewing. I am sorry that I forget to 
remove something.
   Thanks!

Comment 41

[Chris Waterson]

sr=waterson

Comment 42

[Rich Burridge]

Fix checked into the trunk and the OEM branch.

Comment 43

[edburns]

Hi Rich,

Can we get this checked into the Trunk as well?

Comment 44

[Rich Burridge]

Hi Ed. Please re-read my last comment again. ... You're welcome.

Comment 45

[edburns]

Hi Rich, Xiaobin and George,

Thanks for checking in my workaround for the bug in the java plugin.  I know it 
wasn't the most straightforward bug, and it's difficult to test, but that's why 
it's called a showstopper, I guess.

Thanks again.

Ed

Comment 46

[John Unruh]

Verified.