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)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla37

People

(Reporter: ckerschb, Assigned: ckerschb)

References

Details

Attachments

(1 file, 2 obsolete files)

      No description provided.
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: nobody → mozilla
Status: NEW → ASSIGNED
Attached patch bug_1113323_deref_loadinfo.patch (obsolete) — Splinter Review
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)
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-
(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
(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.
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.
Jonas, we should fix this before the train leaves on Monday.
Attachment #8538716 - Attachment is obsolete: true
Attachment #8546025 - Flags: review?(jonas)
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+
https://hg.mozilla.org/mozilla-central/rev/922d760d0dcf
Status: ASSIGNED → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
Depends on: 1131455
You need to log in before you can comment on or make changes to this bug.