537670 - Failure to load pages using Basic Auth and lots of content, after setting the LOAD_ANONYMOUS flag

Status: RESOLVED FIXED

(Core :: Networking: HTTP, defect)

Description

[Philippe De Ryck]

User-Agent:       Mozilla/5.0 (X11; U; Linux x86_64; en-US; rv:1.9.1.6) Gecko/20091216 Fedora/3.5.6-1.fc12 Firefox/3.5.6
Build Identifier: Mozilla/5.0 (X11; U; Linux x86_64; en-US; rv:1.9.1.6) Gecko/20091216 Fedora/3.5.6-1.fc12 Firefox/3.5.6

In my extension, I occasionally set the LOAD_ANONYMOUS flag for a connection. If this connection is used to load pages with basic authentication, it stops loading halfway (for a large page, small pages show no visual problems).

After doing this, closing firefox causes a segmentation fault.

Reproducible: Always

Steps to Reproduce:
1. Start Firefox
2. Load a page requiring Basic Auth from the browser chrome (bookmarks, address bar)
3. Enter credentials
3a. The LOAD_ANONYMOUS flag is set before the reload request is sent out
3b. Send the request with the basic authentication attached
3c. Unset the LOAD_ANONYMOUS flag upon receiving the response
3d. The rest of the page is loaded (in my case, only the favicon requests)
4. Shut down firefox
5. --> segfault

The LOAD_ANONYMOUS flag is set by my extension
Actual Results:  
Firefox segfaults on closing. For large web pages, it loads half of the content and then stops.

Expected Results:  
Loading the entire page and not segfaulting

While developing an extension, I encountered a problem using the LOAD_ANONYMOUS flag while executing the following scenario:

1. Start Firefox
2. Load a page requiring Basic Auth from the browser chrome (bookmarks, address bar)
3. Enter credentials
3a. The LOAD_ANONYMOUS flag is set before the reload request is sent out
3b. Send the request with the basic authentication attached
3c. Unset the LOAD_ANONYMOUS flag upon receiving the response
3d. The rest of the page is loaded (in my case, only the favicon requests)
4. Shut down firefox
5. --> segfault

This scenario creates the following HTTP requests:
1. GET page --> 401
2. GET page with auth --> 200
3. GET favicon --> 404
4. GET favicon --> 404

The segfault is caused due to Firefox cleaning up remaining HTTP connections (nsHttpConnection). For this scenario, two nsConnectionEntry objects have been created (one with the key ...test:80 and one with ..Atest:80). The following happens, based on the HTTP requests discussed above:

1. new nsConnectionEntry a is created with nsConnectionInfo x
2. channel becomes anonymous, new nsConnectionEntry b is created with nsConnectionInfo x
3. channel is not anonymous, nsConnectionEntry a is retrieved, but its nsConnectionInfo x has the anonymous flag
3a. Based on nsConnectionInfo x, nsConnectionEntry y is selected
3b. The current connection (which is part of nsConnectionEntry x) is removed from y's active connections (not present, but no error is shown) and added to y's idle connections
==> The connection is now part of two nsConnectionEntryObjects
4. Repeat favicon request, no issues
5. Shutdown firefox, which closes all connections left in nsConnectionEntry objects
5a. Succeeds for x
5b. Fails for y, since the shared connection already has been destroyed

My suggestion to fix this is to make sure the correct nsConnectionEntry is selected at all times. To do this, the nsConnectionInfo must be independent of the one stored in the nsHttpChannel. Therefore, I've cloned the nsConnectionInfo before creating the nsConnectionEntry. I don't think this has any consequences, since nsConnectionInfo returns a hash key, based on its internal information (no object comparison is done). A patch for this fix is included.

Comment 1

[Philippe De Ryck]

Attachment: Patch that fixes the bug by cloning the HttpConnectionInfo

This patch fixes the bug, but I don't really know what else it affects ...

Comment 2

[Boris Zbarsky [:bzbarsky]]

Jason, Honza, can you look at this?

Comment 3

[Honza Bambas (:mayhemer)]

Looking at this, need a little thinking. Philippe, thanks for a detailed report and a patch.

Comment 4

[Honza Bambas (:mayhemer)]

Philippe, can you describe more in-detail the problem with page load failure? You claim that not all content of a single request is downloaded or not all requests invoked by the page are processed?

Then I don't fully understand what exactly your extension is supposed to do. The anon flag will prevent sending the authorization headers, then you should never get 200 in step 2 of the request list (from the description).

This bug is a regression from bug 474038. That made connection info object mutable. The underlying code was not prepared for this and I assumed it doesn't have to as the anonymous flag will be set just ones for the whole channel life time; wrong assumption.

Preliminarily I agree with the idea of your patch, just:

>+    //Make sure the anonymous flag is transferred!
Please add space between // and Make.

I'm not sure the Clone method should be const.

You should do OOM (non-null) check of the result of ci->Clone() in nsHttpConnectionMgr::ProcessNewTransaction. The same in nsHttpConnectionInfo::Clone for clone local var.


I have to patch the connection info object itself, the flag must be undo-able as well. Will have it soon.

After you test it with your extension and your patch we can go through reviews.

Comment 5

[Honza Bambas (:mayhemer)]

Attachment: nsConnectionInfo object fix, v1

Allowing the flag be dropped.

Comment 6

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 421327 [details] [diff] [review]
nsConnectionInfo object fix, v1

Make it |(mLoadFlags & LOAD_ANONYMOUS) != 0| so you're not passing some random non-0 and non-1 value as PRBool, and r=me.

As for the other patch, I assume you double-checked that we don't leak things?  Where is the clone deallocated?

Comment 7

[Philippe De Ryck]

(In reply to comment #4)
> Philippe, can you describe more in-detail the problem with page load failure?
> You claim that not all content of a single request is downloaded or not all
> requests invoked by the page are processed?
> 
> Then I don't fully understand what exactly your extension is supposed to do.
> The anon flag will prevent sending the authorization headers, then you should
> never get 200 in step 2 of the request list (from the description).

Honza,

thanks for taking the time to look at my bug report. Let me first explain what my extension does: if it detects that a request is cross-domain (which can be potentially dangerous), it may decide to strip authentication info (cookies or auth headers). Cookies are stripped manually (Setting the header info), but auth information is stripped using the LOAD_ANONYMOUS flag. This will cause a 401, which is handled by my extension (it will cause the user to re-enter his credentials), after which a 200 is received. All the contents of the page (CSS, images, ...) is not cross-domain anymore, which means that is not touched by my extension.

The problem first occurred to me when loading a large page, protected by basic auth. It stopped loading halfway, for some reason. After reducing my test case, I discovered that a page containing only the word "test" loads fine, but closing FF leads to a crash. I have investigated this case and solved it with the patch. After fixing the issue, the troubles with the large page were also fixed.

I will fix and test my patch asap

Comment 8

[Philippe De Ryck]

I have updated my patch (attachment following):

Space after comment: fixed

OOM checks: fixed (please check this carefully!)

Clone const: I think it's good to make it const, since it does not modify the object about to be cloned. If you disagree, feel free to remove the const :)

Leak prevention: I believe the cloned instance of nsConnectionInfo (which has no other refs) is destroyed when the nsConnectionEntry is removed:
nsHttpConnectionMgr.h 157:  ~nsConnectionEntry() { NS_RELEASE(mConnInfo); }
 

I have tested it on mozilla 1.9.1 (FF 3.5.*, Shiretoko) and mozilla-center (FF 3.7, Minefield)

Comment 9

[Philippe De Ryck]

Attachment: Revised patch fixing the problem by cloning the HttpConnectionInfo

Updated patch

Comment 10

[Boris Zbarsky [:bzbarsky]]

Ah, connection info is refcounted.  Great.

Comment 11

[Honza Bambas (:mayhemer)]

Comment on attachment 421452 [details] [diff] [review]
Revised patch fixing the problem by cloning the HttpConnectionInfo

>+++ b/netwerk/protocol/http/src/nsHttpConnectionInfo.cpp	Wed Jan 13 15:55:18 2010 +0100
>+nsHttpConnectionInfo*
>+nsHttpConnectionInfo::Clone() const
>+{

>+    nsHttpConnectionInfo* clone = new nsHttpConnectionInfo(mHost, Port(), mProxyInfo, mUsingSSL);

Please clone mPort and not Port(). The getter might change in the future and return something different from mPort itself.

>+    if(!clone)
>+        return 0;

Please return nsnull instead of 0.

>+++ b/netwerk/protocol/http/src/nsHttpConnectionMgr.cpp	Wed Jan 13 15:55:18 2010 +0100
>     nsConnectionEntry *ent = (nsConnectionEntry *) mCT.Get(&key);
>     if (!ent) {
>-        ent = new nsConnectionEntry(ci);
>+        nsHttpConnectionInfo *clone = ci->Clone();
>+        NS_ENSURE_TRUE(clone, NS_ERROR_OUT_OF_MEMORY);

Please keep this way of the check, it complies with the rest of the file:

>         if (!clone)
>             return NS_ERROR_OUT_OF_MEMORY;

Otherwise good.

Comment 12

[Honza Bambas (:mayhemer)]

Attachment: v1.1 consolidated patch [Checkin comment 15]

Ready to land all review comments addressed.

Comment 13

[Philippe De Ryck]

thanks, looks good. Also thank you for the review comments! In which version of FF can I expect the patch to appear? 3.5.8 or 3.7?

Comment 14

[Honza Bambas (:mayhemer)]

Don't expect this get to 3.5.x.

Comment 15

[Honza Bambas (:mayhemer)]

Comment on attachment 421856 [details] [diff] [review]
v1.1 consolidated patch [Checkin comment 15]

http://hg.mozilla.org/mozilla-central/rev/eb9ae14db295