Closed Bug 1487085 Opened Last year Closed Last year

AddressSanitizer: SEGV /builds/worker/workspace/build/src/netwerk/cookie/CookieServiceChild.cpp in mozilla::net::CookieServiceChild::SetCookieStringInternal(nsIURI*, nsIChannel*, char const*, char const*, bool)

Categories

(Core :: Networking: Cookies, defect, P2, critical)

defect

Tracking

()

VERIFIED FIXED
mozilla64
Tracking Status
firefox-esr60 --- wontfix
firefox62 --- wontfix
firefox63 --- verified
firefox64 --- verified

People

(Reporter: jkratzer, Assigned: kershaw)

References

(Blocks 1 open bug)

Details

(Keywords: crash, testcase, Whiteboard: [necko-triaged])

Attachments

(2 files)

Attached file testcase.html
Testcase found while fuzzing mozilla-central rev b75561ff5ffe.

==23620==ERROR: AddressSanitizer: SEGV on unknown address 0x000000000000 (pc 0x7f314f217b8f bp 0x7ffd7eac11f0 sp 0x7ffd7eac07c0 T0)
==23620==The signal is caused by a READ memory access.
==23620==Hint: address points to the zero page.
    #0 0x7f314f217b8e in mozilla::net::CookieServiceChild::SetCookieStringInternal(nsIURI*, nsIChannel*, char const*, char const*, bool) /builds/worker/workspace/build/src/netwerk/cookie/CookieServiceChild.cpp
    #1 0x7f3152dd96e6 in nsContentSink::ProcessHeaderData(nsAtom*, nsTSubstring<char16_t> const&, nsIContent*) /builds/worker/workspace/build/src/dom/base/nsContentSink.cpp:341:22
    #2 0x7f3152de0699 in nsContentSink::ProcessMETATag(nsIContent*) /builds/worker/workspace/build/src/dom/base/nsContentSink.cpp:845:12
    #3 0x7f31517dc857 in nsHtml5TreeOperation::Perform(nsHtml5TreeOpExecutor*, nsIContent**, bool*, bool*) /builds/worker/workspace/build/src/parser/html/nsHtml5TreeOperation.cpp:1012:24
    #4 0x7f31517e0c05 in nsHtml5TreeOpExecutor::FlushDocumentWrite() /builds/worker/workspace/build/src/parser/html/nsHtml5TreeOpExecutor.cpp:604:18
    #5 0x7f315174400d in nsHtml5Parser::Parse(nsTSubstring<char16_t> const&, void*, nsTSubstring<char> const&, bool, nsDTDMode) /builds/worker/workspace/build/src/parser/html/nsHtml5Parser.cpp:461:20
    #6 0x7f3156b41b9b in nsHTMLDocument::WriteCommon(JSContext*, nsTSubstring<char16_t> const&, bool, mozilla::ErrorResult&) /builds/worker/workspace/build/src/dom/html/nsHTMLDocument.cpp:1804:57
    #7 0x7f3156b40980 in nsHTMLDocument::WriteCommon(JSContext*, mozilla::dom::Sequence<nsTString<char16_t> > const&, bool, mozilla::ErrorResult&) /builds/worker/workspace/build/src/dom/html/nsHTMLDocument.cpp:1686:5
    #8 0x7f31558048a3 in mozilla::dom::HTMLDocument_Binding::write(JSContext*, JS::Handle<JSObject*>, nsHTMLDocument*, JSJitMethodCallArgs const&) /builds/worker/workspace/build/src/obj-firefox/dom/bindings/HTMLDocumentBinding.cpp:367:9
    #9 0x7f3155db87a9 in bool mozilla::dom::binding_detail::GenericMethod<mozilla::dom::binding_detail::NormalThisPolicy, mozilla::dom::binding_detail::ThrowExceptions>(JSContext*, unsigned int, JS::Value*) /builds/worker/workspace/build/src/dom/bindings/BindingUtils.cpp:3296:13
    #10 0x7f315d0a975b in CallJSNative /builds/worker/workspace/build/src/js/src/vm/Interpreter.cpp:449:15
    #11 0x7f315d0a975b in js::InternalCallOrConstruct(JSContext*, JS::CallArgs const&, js::MaybeConstruct) /builds/worker/workspace/build/src/js/src/vm/Interpreter.cpp:537
    #12 0x7f315d0930a3 in CallFromStack /builds/worker/workspace/build/src/js/src/vm/Interpreter.cpp:594:12
    #13 0x7f315d0930a3 in Interpret(JSContext*, js::RunState&) /builds/worker/workspace/build/src/js/src/vm/Interpreter.cpp:3266
    #14 0x7f315d078c2e in js::RunScript(JSContext*, js::RunState&) /builds/worker/workspace/build/src/js/src/vm/Interpreter.cpp:429:12
    #15 0x7f315d0aa26e in js::InternalCallOrConstruct(JSContext*, JS::CallArgs const&, js::MaybeConstruct) /builds/worker/workspace/build/src/js/src/vm/Interpreter.cpp:561:15
    #16 0x7f315d0ac002 in js::Call(JSContext*, JS::Handle<JS::Value>, JS::Handle<JS::Value>, js::AnyInvokeArgs const&, JS::MutableHandle<JS::Value>) /builds/worker/workspace/build/src/js/src/vm/Interpreter.cpp:607:10
    #17 0x7f315db430bd in JS::Call(JSContext*, JS::Handle<JS::Value>, JS::Handle<JS::Value>, JS::HandleValueArray const&, JS::MutableHandle<JS::Value>) /builds/worker/workspace/build/src/js/src/jsapi.cpp:2927:12
    #18 0x7f31553bd56e in mozilla::dom::EventListener::HandleEvent(JSContext*, JS::Handle<JS::Value>, mozilla::dom::Event&, mozilla::ErrorResult&) /builds/worker/workspace/build/src/obj-firefox/dom/bindings/EventListenerBinding.cpp:51:8
    #19 0x7f315662aaae in HandleEvent<mozilla::dom::EventTarget *> /builds/worker/workspace/build/src/obj-firefox/dist/include/mozilla/dom/EventListenerBinding.h:66:12
    #20 0x7f315662aaae in mozilla::EventListenerManager::HandleEventSubType(mozilla::EventListenerManager::Listener*, mozilla::dom::Event*, mozilla::dom::EventTarget*) /builds/worker/workspace/build/src/dom/events/EventListenerManager.cpp:1108
    #21 0x7f315662cc37 in mozilla::EventListenerManager::HandleEventInternal(nsPresContext*, mozilla::WidgetEvent*, mozilla::dom::Event**, mozilla::dom::EventTarget*, nsEventStatus*, bool) /builds/worker/workspace/build/src/dom/events/EventListenerManager.cpp:1342:20
    #22 0x7f31566106c9 in HandleEvent /builds/worker/workspace/build/src/obj-firefox/dist/include/mozilla/EventListenerManager.h:390:5
    #23 0x7f31566106c9 in mozilla::EventTargetChainItem::HandleEvent(mozilla::EventChainPostVisitor&, mozilla::ELMCreationDetector&) /builds/worker/workspace/build/src/dom/events/EventDispatcher.cpp:420
    #24 0x7f315660e983 in mozilla::EventTargetChainItem::HandleEventTargetChain(nsTArray<mozilla::EventTargetChainItem>&, mozilla::EventChainPostVisitor&, mozilla::EventDispatchingCallback*, mozilla::ELMCreationDetector&) /builds/worker/workspace/build/src/dom/events/EventDispatcher.cpp:637:16
    #25 0x7f315661516e in mozilla::EventDispatcher::Dispatch(nsISupports*, nsPresContext*, mozilla::WidgetEvent*, mozilla::dom::Event*, nsEventStatus*, mozilla::EventDispatchingCallback*, nsTArray<mozilla::dom::EventTarget*>*) /builds/worker/workspace/build/src/dom/events/EventDispatcher.cpp:1112:9
    #26 0x7f3156618516 in mozilla::EventDispatcher::DispatchDOMEvent(nsISupports*, mozilla::WidgetEvent*, mozilla::dom::Event*, nsPresContext*, nsEventStatus*) /builds/worker/workspace/build/src/dom/events/EventDispatcher.cpp
    #27 0x7f3152f89dd4 in nsINode::DispatchEvent(mozilla::dom::Event&, mozilla::dom::CallerType, mozilla::ErrorResult&) /builds/worker/workspace/build/src/dom/base/nsINode.cpp:1110:5
    #28 0x7f315297d489 in nsContentUtils::DispatchEvent(nsIDocument*, nsISupports*, nsTSubstring<char16_t> const&, mozilla::CanBubble, mozilla::Cancelable, mozilla::Trusted, bool*, mozilla::ChromeOnlyDispatch) /builds/worker/workspace/build/src/dom/base/nsContentUtils.cpp:4439:28
    #29 0x7f315297d219 in nsContentUtils::DispatchTrustedEvent(nsIDocument*, nsISupports*, nsTSubstring<char16_t> const&, mozilla::CanBubble, mozilla::Cancelable, bool*) /builds/worker/workspace/build/src/dom/base/nsContentUtils.cpp:4403:10
    #30 0x7f3152e89332 in nsIDocument::DispatchContentLoadedEvents() /builds/worker/workspace/build/src/dom/base/nsDocument.cpp:4992:3
    #31 0x7f3152ff19fb in applyImpl<nsIDocument, void (nsIDocument::*)()> /builds/worker/workspace/build/src/obj-firefox/dist/include/nsThreadUtils.h:1168:12
    #32 0x7f3152ff19fb in apply<nsIDocument, void (nsIDocument::*)()> /builds/worker/workspace/build/src/obj-firefox/dist/include/nsThreadUtils.h:1174
    #33 0x7f3152ff19fb in mozilla::detail::RunnableMethodImpl<nsIDocument*, void (nsIDocument::*)(), true, (mozilla::RunnableKind)0>::Run() /builds/worker/workspace/build/src/obj-firefox/dist/include/nsThreadUtils.h:1219
    #34 0x7f314ede9fa2 in mozilla::SchedulerGroup::Runnable::Run() /builds/worker/workspace/build/src/xpcom/threads/SchedulerGroup.cpp:337:32
    #35 0x7f314ee26e70 in nsThread::ProcessNextEvent(bool, bool*) /builds/worker/workspace/build/src/xpcom/threads/nsThread.cpp:1167:14
    #36 0x7f314ee2fb85 in NS_ProcessNextEvent(nsIThread*, bool) /builds/worker/workspace/build/src/xpcom/threads/nsThreadUtils.cpp:519:10
    #37 0x7f3150011dce in mozilla::ipc::MessagePump::Run(base::MessagePump::Delegate*) /builds/worker/workspace/build/src/ipc/glue/MessagePump.cpp:97:21
    #38 0x7f314ff13a1c in RunInternal /builds/worker/workspace/build/src/ipc/chromium/src/base/message_loop.cc:325:10
    #39 0x7f314ff13a1c in RunHandler /builds/worker/workspace/build/src/ipc/chromium/src/base/message_loop.cc:318
    #40 0x7f314ff13a1c in MessageLoop::Run() /builds/worker/workspace/build/src/ipc/chromium/src/base/message_loop.cc:298
    #41 0x7f31589f2206 in nsBaseAppShell::Run() /builds/worker/workspace/build/src/widget/nsBaseAppShell.cpp:158:27
    #42 0x7f315cd8291e in XRE_RunAppShell() /builds/worker/workspace/build/src/toolkit/xre/nsEmbedFunctions.cpp:944:22
    #43 0x7f314ff13a1c in RunInternal /builds/worker/workspace/build/src/ipc/chromium/src/base/message_loop.cc:325:10
    #44 0x7f314ff13a1c in RunHandler /builds/worker/workspace/build/src/ipc/chromium/src/base/message_loop.cc:318
    #45 0x7f314ff13a1c in MessageLoop::Run() /builds/worker/workspace/build/src/ipc/chromium/src/base/message_loop.cc:298
    #46 0x7f315cd819d5 in XRE_InitChildProcess(int, char**, XREChildData const*) /builds/worker/workspace/build/src/toolkit/xre/nsEmbedFunctions.cpp:770:34
    #47 0x4f5b21 in content_process_main /builds/worker/workspace/build/src/browser/app/../../ipc/contentproc/plugin-container.cpp:50:30
    #48 0x4f5b21 in main /builds/worker/workspace/build/src/browser/app/nsBrowserApp.cpp:287
    #49 0x7f3171e71b96 in __libc_start_main /build/glibc-OTsEL5/glibc-2.27/csu/../csu/libc-start.c:310
Flags: in-testsuite?
Kershaw, do you have time to look at this?
Flags: needinfo?(kershaw)
(In reply to Dragana Damjanovic [:dragana] from comment #1)
> Kershaw, do you have time to look at this?

Sure.
Assignee: nobody → kershaw
Flags: needinfo?(kershaw)
Priority: -- → P2
Whiteboard: [necko-triaged]
It looks like the problem is at [1]. |aChannel| could be null in this function.
I am thinking maybe we can use |aHostURI| as channel uri when |aChannel| is null.

:ckerschb, what do you think?


[1] https://searchfox.org/mozilla-central/rev/721842eed881c7fcdccb9ec0fe79e4e6d4e46604/netwerk/cookie/CookieServiceChild.cpp#669
Flags: needinfo?(ckerschb)
(In reply to Kershaw Chang [:kershaw] from comment #3)
> It looks like the problem is at [1]. |aChannel| could be null in this
> function.
> I am thinking maybe we can use |aHostURI| as channel uri when |aChannel| is
> null.
> 
> :ckerschb, what do you think?

Personally I think we could just do |if (!aChannel) {return NS_OK}| similar do what we do for moz-nullprincipals.

If returning is not the right approach then I would prefer to if-check that aChannel is not null before querying the channelURI and just pass empty channelURIParams in that case.

Valentin, what do you prefer?

> [1]
> https://searchfox.org/mozilla-central/rev/
> 721842eed881c7fcdccb9ec0fe79e4e6d4e46604/netwerk/cookie/CookieServiceChild.
> cpp#669
Flags: needinfo?(ckerschb) → needinfo?(valentin.gosu)
(In reply to Christoph Kerschbaumer [:ckerschb] from comment #4)
> (In reply to Kershaw Chang [:kershaw] from comment #3)
> > It looks like the problem is at [1]. |aChannel| could be null in this
> > function.
> > I am thinking maybe we can use |aHostURI| as channel uri when |aChannel| is
> > null.
> > :ckerschb, what do you think?
> Personally I think we could just do |if (!aChannel) {return NS_OK}| similar
> do what we do for moz-nullprincipals.
> If returning is not the right approach then I would prefer to if-check that
> aChannel is not null before querying the channelURI and just pass empty
> channelURIParams in that case.
> 
> Valentin, what do you prefer?

So, from what I can tell the problem is that setting a cookie via a meta tag will call this method with a null aChannel. (or is it because the tag is added after onLoad? )
Given [1] I think we are going to remove the ability to set cookies via meta tags, so { return NS_OK; } would be best.
In Chrome the testcase also shows a deprecation warning - maybe we should do the same? Let's file a separate bug for that.

[1] https://github.com/whatwg/html/issues/3076
Flags: needinfo?(valentin.gosu)
(In reply to Valentin Gosu [:valentin] from comment #5)
> (In reply to Christoph Kerschbaumer [:ckerschb] from comment #4)
> > (In reply to Kershaw Chang [:kershaw] from comment #3)
> > > It looks like the problem is at [1]. |aChannel| could be null in this
> > > function.
> > > I am thinking maybe we can use |aHostURI| as channel uri when |aChannel| is
> > > null.
> > > :ckerschb, what do you think?
> > Personally I think we could just do |if (!aChannel) {return NS_OK}| similar
> > do what we do for moz-nullprincipals.
> > If returning is not the right approach then I would prefer to if-check that
> > aChannel is not null before querying the channelURI and just pass empty
> > channelURIParams in that case.
> > 
> > Valentin, what do you prefer?
> 
> So, from what I can tell the problem is that setting a cookie via a meta tag
> will call this method with a null aChannel. (or is it because the tag is
> added after onLoad? )
> Given [1] I think we are going to remove the ability to set cookies via meta
> tags, so { return NS_OK; } would be best.
> In Chrome the testcase also shows a deprecation warning - maybe we should do
> the same? Let's file a separate bug for that.
> 
> [1] https://github.com/whatwg/html/issues/3076

I think returning NS_OK when we have a null channel is probably not a good idea.
Looking into the implementation of nsICookieService::SetCookieString in parent process, we do allow to set a cookie with a null channel. I think we should make sure the behavior in e10s mode is consistent to non-e10s.
So, passing empty channelURIParams should be the right approach.
(In reply to Kershaw Chang [:kershaw] from comment #6)
> So, passing empty channelURIParams should be the right approach.

That makes sense. Let's do it.
In the current implmentation of CookieServiceChild::SetCookieString, pass a null channel will crash the child process. This is because we call aChannel->GetURI() without checking if aChannel is null.
However, set cookie with a null channel is possible in non-e10s mode. To make sure the behavior to be consistent in both non-e10s and e10s mode, we have to pass an empty URIParams in child process.
Comment on attachment 9007800 [details]
Bug 1487085 - Allow to set cookie with a null channel in child process, r=valentin

Valentin Gosu [:valentin] has approved the revision.
Attachment #9007800 - Flags: review+
Attachment #9007800 - Attachment description: Bug 1487085 - Allow to set cookie with a null channel in child process → Bug 1487085 - Allow to set cookie with a null channel in child process, r=valentin
Pushed by kjang@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/0d5b78130661
Allow to set cookie with a null channel in child process, r=valentin
Pushed by dluca@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/ac8b78a93527
Allow to set cookie with a null channel in child process, r=valentin
https://hg.mozilla.org/mozilla-central/rev/0d5b78130661
Status: NEW → RESOLVED
Closed: Last year
Resolution: --- → FIXED
Target Milestone: --- → mozilla64
P2 crash fix, Kershaw, could your request an uplift to beta when you get a chance? Thanks
Flags: needinfo?(kershaw)
Flags: in-testsuite? → in-testsuite-
Comment on attachment 9007800 [details]
Bug 1487085 - Allow to set cookie with a null channel in child process, r=valentin

Approval Request Comment
[Feature/Bug causing the regression]:
Bug 1452496

[User impact if declined]:
The tab will crash when setting a cookie via meta tag.

[Is this code covered by automated tests?]:
No.

[Has the fix been verified in Nightly?]:
Yes.

[Needs manual test from QE? If yes, steps to reproduce]:
Just open the attached testcase.html and see if the tab crashed.

[List of other uplifts needed for the feature/fix]:
None.

[Is the change risky?]:
No.

[Why is the change risky/not risky?]:
This patch just adds a null check.

[String changes made/needed]:
No.
Flags: needinfo?(kershaw)
Attachment #9007800 - Flags: approval-mozilla-beta?
Comment on attachment 9007800 [details]
Bug 1487085 - Allow to set cookie with a null channel in child process, r=valentin

Low risk patch fixing a crash, approved for 63 beta 6. Thanks
Attachment #9007800 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
I can confirm the reproduction of the issue in Nightly from 2018-08-29 and I can confirm the fix in Nightly from 2018-09-13. Awaiting the Beta build for uplift verification. Thank you.
I have reproduced the issue in Beta v63.0b5 and I have verified the fix in Beta v63.0b6 on the 3 main OSes (Windows 10, Mac OS X 10.13.6, Ubuntu 18.04) with the test case provided in comment 0.

Uplift successful. Thank you!
Status: RESOLVED → VERIFIED
Flags: qe-verify+
You need to log in before you can comment on or make changes to this bug.