Closed
Bug 1113323
Opened 9 years ago
Closed 9 years ago
Don't dereference loadInfo, but rather aLoadInfo in nsIoService
Categories
(Core :: DOM: Security, defect)
Core
DOM: Security
Tracking
()
RESOLVED
FIXED
mozilla37
People
(Reporter: ckerschb, Assigned: ckerschb)
References
Details
Attachments
(1 file, 2 obsolete files)
2.90 KB,
patch
|
ckerschb
:
review+
|
Details | Diff | Splinter Review |
No description provided.
Assignee | ||
Comment 1•9 years ago
|
||
Thanks Patrick for pointing out that we should rather dereference aLoadInfo instead of loadinfo in nsioservice.cpp: http://mxr.mozilla.org/mozilla-central/source/netwerk/base/src/nsIOService.cpp#691
Blocks: 1087442
Assignee | ||
Updated•9 years ago
|
Assignee: nobody → mozilla
Status: NEW → ASSIGNED
Assignee | ||
Comment 2•9 years ago
|
||
Jonas, there might be cases where we end up not having a loadInfo on the channel. Currently we do a MOZ_ASSERT(loadInfo), but right after we deref that pointer, which is no good! In fact, we should check that channel actually has a loadInfo, and only if there is a loadInfo, and the sandbox flag is set, then we should set the owner to null.
Attachment #8538716 -
Flags: review?(jonas)
Attachment #8538716 -
Flags: review?(jonas) → review+
Assignee | ||
Comment 3•9 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/5e4392cb16c6
Target Milestone: --- → mozilla37
Comment on attachment 8538716 [details] [diff] [review] bug_1113323_deref_loadinfo.patch Review of attachment 8538716 [details] [diff] [review]: ----------------------------------------------------------------- Actually, I don't think we should do this. Anyone implementing NewChannel2 should always set a loadinfo. It's actually good that we crash early so that if they forget they find out early. Otherwise we might end up having to nullcheck in a lot more places.
Attachment #8538716 -
Flags: review+ → review-
Assignee | ||
Comment 5•9 years ago
|
||
(In reply to Jonas Sicking (:sicking) from comment #4) > Comment on attachment 8538716 [details] [diff] [review] > bug_1113323_deref_loadinfo.patch > > Review of attachment 8538716 [details] [diff] [review]: > ----------------------------------------------------------------- > > Actually, I don't think we should do this. Anyone implementing NewChannel2 > should always set a loadinfo. It's actually good that we crash early so that > if they forget they find out early. Otherwise we might end up having to > nullcheck in a lot more places. Sorry Jonas for the mis-communication here. I backed the patch out: https://hg.mozilla.org/integration/mozilla-inbound/rev/00da5b2f2bcb
Comment 6•9 years ago
|
||
(In reply to Jonas Sicking (:sicking) from comment #4) > Comment on attachment 8538716 [details] [diff] [review] > bug_1113323_deref_loadinfo.patch > > Review of attachment 8538716 [details] [diff] [review]: > ----------------------------------------------------------------- > > Actually, I don't think we should do this. Anyone implementing NewChannel2 > should always set a loadinfo. It's actually good that we crash early so that > if they forget they find out early. Otherwise we might end up having to > nullcheck in a lot more places. jonas - you kind of lost me on this one. If someone screws up by not setting loading, we should throw an exception not segv right? (its ok to MOZ_ASSERT it for debug builds, I'm talking release) so the right patch might say if (!loadinfo) { MOZ_ASSERT(false); NS_WARNING("you suck"); reutrn NS_ERROR_BLAH; } do we crash anywhere else (on purpose :)) when an addon misuses an idl?
Actually, what we probably should do here is to make getting the loadinfo be debug-only, and then assert that the returned loadinfo is == aLoadInfo. Then in the non-debug code use aLoadInfo to check if the sandbox flag has been set.
> jonas - you kind of lost me on this one. If someone screws up by not setting > loading, we should throw an exception not segv right? (its ok to MOZ_ASSERT > it for debug builds, I'm talking release) I was recommending segv. > do we crash anywhere else (on purpose :)) when an addon misuses an idl? All the time! We generally expect addons to honor our idl contracts. We're essentially forced to because we allow addons to modify lots and lots of our internal interfaces. If we didn't want to rely on that addons do the right thing when implementing those internal interfaces, we would be sprinkling null-checks and other runtime tests all over the place.
Comment 9•9 years ago
|
||
I think this becomes a discussion of how deep into an interface you should do the precondition checking. I still think that this is basically something that is detected in the ioservice failing to create a channel and an exception is a better path here. Is a total crash really the right path for someone that has screwed up their protocol implementation - failing to use the protocol seems pretty noticeable to me. so that's my pov. But on this matter if I fail to convince you, I'm ok with being in the rough. If that's the case, let's change the code to MOZ_CRASH instead of just derf'ing the null pointer so that the intent is clear.
Indeed, returning "unable to create channel" is likely going to work as well as crashing in order to alert the developer that they've done the wrong thing. I guess my general rule of thumb is to not write lots of extra code in order to "fail nicely" in the face of buggy addons that inject themselves deep into XPCOM. But "lots of extra code" is most certainly subjective. In this case, I think we can simply go with the approach in comment 7, in which case the segv would go away.
Assignee | ||
Comment 11•9 years ago
|
||
Jonas, we should fix this before the train leaves on Monday.
Attachment #8538716 -
Attachment is obsolete: true
Attachment #8546025 -
Flags: review?(jonas)
Attachment #8546025 -
Flags: review?(jonas) → review+
Assignee | ||
Comment 12•9 years ago
|
||
Since we want the same instance of loadInfo set on the channel in the protocolHandler we have to update one test with implements nsIAboutModule in the test itself. Carrying over r+ from Jonas.
Attachment #8546025 -
Attachment is obsolete: true
Attachment #8547331 -
Flags: review+
Assignee | ||
Comment 13•9 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/922d760d0dcf
Comment 14•9 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/922d760d0dcf
Status: ASSIGNED → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
You need to log in
before you can comment on or make changes to this bug.
Description
•