Add assertion to AsyncOpen to make sure AsyncOpen(2) was called first when securityflags in loadInfo are set

RESOLVED FIXED in Firefox 43

Status

()

RESOLVED FIXED
3 years ago
3 years ago

People

(Reporter: ckerschb, Assigned: ckerschb)

Tracking

unspecified
mozilla43
Points:
---

Firefox Tracking Flags

(firefox43 fixed)

Details

Attachments

(1 attachment, 1 obsolete attachment)

(Assignee)

Description

3 years ago
We should land assertions to make sure that asyncOpen(2) is always called before asyncOpen(). We can make sure that happenened by investigating the 'securtyMode' within the Loadinfo to determine if asyncOpen(2) should have been called. If so, we can check that 'initialSecurityChecksDone' flag is set.

I think it's particularly relevant now that we are getting into callsites where channel creation and opening happens at different places.
(Assignee)

Updated

3 years ago
Assignee: nobody → mozilla
Blocks: 1182535
(Assignee)

Comment 1

3 years ago
Created attachment 8647121 [details] [diff] [review]
bug_1182569_asyncopen2_assertions.patch

Do you really have to assign an already_addrefed value to a temprorary first, before you can use it? E.g in the case of nsJSChannel::AsyncOpen, do I really have to assign the loadInfo to a tmp variable? It seems, all over the codebase, this is what we do :-(

Also, I think we may want to also check for mLoadInfo first, because if an addon calls newChannel instead of newChannel2, then the channel does not have a loadInfo. So the assertion looks something like this:

> MOZ_ASSERT(mLoadInfo || mLoadInfo->GetSecurityMode() == 0 ||
>            mLoadInfo->GetInitialSecurityCheckDone(),
>             "security flags in loadInfo but asyncOpen(2) not called");

What do you think Jonas? Do we still care about the case where the channel might not have a loadInfo??
Attachment #8647121 - Flags: review?(jonas)
> Do you really have to assign an already_addrefed value to a temprorary
> first, before you can use it? E.g in the case of nsJSChannel::AsyncOpen, do
> I really have to assign the loadInfo to a tmp variable? It seems, all over
> the codebase, this is what we do :-(

If you didn't assign to a temporary, you'd leak. So yes, you have to assign an already_AddRefed before you can use it.

But I don't know why you are use

    nsCOMPtr<nsILoadInfo> loadInfo;
    GetLoadInfo(getter_AddRefs(loadInfo));

rather than

    nsCOMPtr<nsILoadInfo> loadInfo = GetLoadInfo();

?

> Also, I think we may want to also check for mLoadInfo first, because if an
> addon calls newChannel instead of newChannel2, then the channel does not
> have a loadInfo. So the assertion looks something like this:
> 
> > MOZ_ASSERT(mLoadInfo || mLoadInfo->GetSecurityMode() == 0 ||
> >            mLoadInfo->GetInitialSecurityCheckDone(),
> >             "security flags in loadInfo but asyncOpen(2) not called");
> 
> What do you think Jonas? Do we still care about the case where the channel
> might not have a loadInfo??

Yes. We still care about addons creating channels. I don't know why anything would have changed here?

However, your code above gets the nullcheck wrong. What you want is

MOZ_ASSERT(!mLoadInfo || mLoadInfo->GetSecurityMode() == 0 ||
           mLoadInfo->GetInitialSecurityCheckDone(),
            "security flags in loadInfo but asyncOpen(2) not called");
(Assignee)

Comment 3

3 years ago
Created attachment 8647307 [details] [diff] [review]
bug_1193924_asyncopen_assertions.patch

(In reply to Jonas Sicking (:sicking) from comment #2)
> > What do you think Jonas? Do we still care about the case where the channel
> > might not have a loadInfo??
> 
> Yes. We still care about addons creating channels. I don't know why anything
> would have changed here?

I updated the assertion to account for a null loadInfo. What I meant is, do we still care about addon code for debug builds, because in release builds they would work just fine. Anyway, I think it makes the most sense to make the assertion account for a null loadInfo.


Further, there are 2 cases where we have to use a slightly different assertion, namely in
* nsJARChannel::AsyncOpen
* nsViewSourceChannel::AsyncOpen

In those two cases the outer channel does not perform security checks in AsyncOpen(2), but rather sets the 'EnforceSecurityFlag'. I think it still makes sense to assert on that in those two cases if the securityMode is set in the LoadInfo.
Attachment #8647121 - Attachment is obsolete: true
Attachment #8647307 - Flags: review?(jonas)
Comment on attachment 8647307 [details] [diff] [review]
bug_1193924_asyncopen_assertions.patch

Review of attachment 8647307 [details] [diff] [review]:
-----------------------------------------------------------------

r=me with that fixed.

::: dom/jsurl/nsJSProtocolHandler.cpp
@@ +569,5 @@
> +    {
> +    nsCOMPtr<nsILoadInfo> loadInfo = nsIChannel::GetLoadInfo();
> +    MOZ_ASSERT(!loadInfo || loadInfo->GetSecurityMode() == 0 ||
> +               loadInfo->GetInitialSecurityCheckDone(),
> +               "security flags in loadInfo but asyncOpen(2) not called");

Err.. all of these should say asyncOpen2, right? asyncOpen was called.
Attachment #8647307 - Flags: review?(jonas) → review+
(Assignee)

Comment 5

3 years ago
url:        https://hg.mozilla.org/integration/mozilla-inbound/rev/f99b27e40987b982c17fa8271ad94754228f4e92
changeset:  f99b27e40987b982c17fa8271ad94754228f4e92
user:       Christoph Kerschbaumer <mozilla@christophkerschbaumer.com>
date:       Wed Aug 12 21:36:33 2015 -0700
description:
Bug 1193924 - Add assertion to AsyncOpen to make sure asyncOpen2() was called first when securityflags in loadInfo are set (r=sicking)
Make mLoadInfo protected rather than private
(Assignee)

Comment 9

3 years ago
url:        https://hg.mozilla.org/integration/mozilla-inbound/rev/276b3c44eff17b097d3cdc3c1f769597d2aaf62e
changeset:  276b3c44eff17b097d3cdc3c1f769597d2aaf62e
user:       Christoph Kerschbaumer <mozilla@christophkerschbaumer.com>
date:       Wed Aug 12 21:36:33 2015 -0700
description:
Bug 1193924 - Add assertion to AsyncOpen to make sure asyncOpen2() was called first when securityflags in loadInfo are set (r=sicking)
https://hg.mozilla.org/mozilla-central/rev/276b3c44eff1
Status: NEW → RESOLVED
Last Resolved: 3 years ago
status-firefox43: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla43
You need to log in before you can comment on or make changes to this bug.