Closed
Bug 591552
Opened 14 years ago
Closed 14 years ago
SetupReplacementChannel has bogus cast to nsHttpChannel
Categories
(Core :: Networking: HTTP, defect)
Tracking
()
RESOLVED
FIXED
Tracking | Status | |
---|---|---|
blocking2.0 | --- | final+ |
status1.9.2 | --- | unaffected |
status1.9.1 | --- | unaffected |
People
(Reporter: bzbarsky, Assigned: jduell.mcbugs)
References
Details
(Keywords: regression, Whiteboard: [sg:critical?])
Attachments
(1 file, 1 obsolete file)
Bug 570867 added this code to SetupReplacementChannel:
1.64 + nsHttpChannel *httpChannelImpl = static_cast<nsHttpChannel*>(httpChannel.get());
1.65 + httpChannelImpl->SetRemoteChannel(mRemoteChannel);
But all you know about httpChannel is that it's some channel that QIs to nsIHttpChannel. This could be implemented by some extension. Or it could be an nsViewSourceChannel (for a view-source:http://something URI).
It looks to me offhand that a server returning 3xx with a location that points to such a URI will cause us to write the remote-channel boolean at 676 bytes into the channel (on 64-bit, at least, that's the offset of mRemoteChannel in the HTTP channel). sizeof(nsViewSourceChannel) there is 128 bits, so we're just writing it.... somewhere.
We need to fix this.
Reporter | ||
Updated•14 years ago
|
Group: core-security
blocking2.0: --- → final+
Comment 1•14 years ago
|
||
Move remoteChannel attribute to nsIHttpChannelInternal or have a new interface for these remote things that is implemented only with MOZ_IPC?
It seems to me that view source doesn't follow redirects, at least after my local test with Fx 3.6. If SetupRedirectChannel is called still needs to be checked. Of course, just to check severity of this bug.
Assignee | ||
Comment 2•14 years ago
|
||
This fixes the cast by adding SetRemoteChannel to nsIHttpChannelInternel (which is currently only impl'd by HTTP channels, not view-source).
It's not the prettiest thing ever, but I couldn't think of anything better. (I pondered creating a CID for nsHttpChannel and QI-ing it so we could cast safely, but got confused as to how to actually make a concrete class QI-able: nsDocumentURI does it, but it confuses me--it has two CIDs, for instance).
There was also the underlyingChannel idea bz and I have talked about, but it's not clear that we're going to actually need it for redirects to other protocols, and it seemed the wrong fix for just this. But if we wind up using underlyingChannel in the future, we might be able to piggyback on it and get rid of servicingRemoteChannel.
> httpInternal->SetServicingRemoteChannel(mRemoteChannel)
mRemoteChannel is a bitfield, but that's fine to pass to an IDL SetBool function right? (i.e. passing by value, so converts to PR_BOOL ok)
I made HttpChannelChild return an error is Get|Set is called--I could've just always returned PR_FALSE for Get, and have Set drop quietly, but figured I might as well flag it as a logic error.
Attachment #470223 -
Flags: review?(bzbarsky)
Assignee | ||
Comment 3•14 years ago
|
||
Honza, view-source does appear to follow redirects: try
view-source:http://people.mozilla.org/~jduell/redir
But this bug is just for redirects *TO* view-source, i.e. see
http://people.mozilla.org/~jduell/redirvs
[Note that bz pointed out to me that we don't actually handle redirects from HTTP to other protocols correctly in e10s--we always create a new PHttpChannel in HttpChannelParentListener::AsyncOnChannelRedirect. I'm going to open a bug for this--alas, I think it's going to be a big pain to fix.]
Updated•14 years ago
|
status1.9.1:
--- → unaffected
status1.9.2:
--- → unaffected
Keywords: regression
Whiteboard: [sg:critical?]
Updated•14 years ago
|
Assignee: nobody → jduell.mcbugs
Reporter | ||
Comment 4•14 years ago
|
||
Comment on attachment 470223 [details] [diff] [review]
Fixes bogus cast by adding IDL attribute
Please rev the iid (and doing this so late in the cycle worries me... I hope we'll be ok, but it might be safer to do an nsIHttpChannelInternal_MOZILLA_2_0_BRANCH, perhaps).
And no need to null-check your out param.
r=me, but we should seriously think about the nsIHttpChannelInternal_MOZILLA_2_0_BRANCH thing.
Attachment #470223 -
Flags: review?(bzbarsky) → review+
Comment 5•14 years ago
|
||
Yeah, we've had a bunch of trouble resulting from changing nsIChannel in beta5. The likelihood of someone using nsIHttpChannelInternal is lower, but I for one am done with playing with fire. ;)
I'd suggest the branch interface, and file a followup to remove it after.
Assignee | ||
Comment 6•14 years ago
|
||
OK, I've fixed this with a separate IDL interface, nsIHttpChannelParentInternal. Free bonus: since only nsHttpChannel needs to implement it, so no chaff methods in HttpChannelChild. (Perhaps we might even want to keep it around for this reason after the branch?)
Attachment #470223 -
Attachment is obsolete: true
Assignee | ||
Updated•14 years ago
|
Keywords: checkin-needed
Comment 7•14 years ago
|
||
Keywords: checkin-needed
Comment 8•14 years ago
|
||
Status: NEW → RESOLVED
Closed: 14 years ago
Resolution: --- → FIXED
Updated•14 years ago
|
Group: core-security
You need to log in
before you can comment on or make changes to this bug.
Description
•