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

Status: VERIFIED FIXED

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

Description

[Jason Kratzer [:jkratzer]]

Attachment: 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

Comment 1

[Dragana Damjanovic [:dragana]]

Kershaw, do you have time to look at this?

Comment 2

[Kershaw Chang [:kershaw]]

(In reply to Dragana Damjanovic [:dragana] from comment #1)
> Kershaw, do you have time to look at this?

Sure.

Comment 3

[Kershaw Chang [:kershaw]]

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

Comment 4

[Christoph Kerschbaumer [: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

Comment 5

[Valentin Gosu [:valentin] (he/him)]

(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

Comment 6

[Kershaw Chang [:kershaw]]

(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.

Comment 7

[Valentin Gosu [:valentin] (he/him)]

(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.

Comment 8

[Kershaw Chang [:kershaw]]

Attachment: Bug 1487085 - Allow to set cookie with a null channel in child process, r=valentin

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 9

[Valentin Gosu [:valentin] (he/him)]

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.

Comment 10

[Kershaw Chang [:kershaw]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=8d8627e71c3c0d4bfb32c542259e4ff6a0d07bdc

Comment 11

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/0d5b78130661
Allow to set cookie with a null channel in child process, r=valentin

Comment 12

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/ac8b78a93527
Allow to set cookie with a null channel in child process, r=valentin

Comment 13

[Daniel Varga [:dvarga]]

https://hg.mozilla.org/mozilla-central/rev/0d5b78130661

Comment 14

[Daniel Varga [:dvarga]]

https://hg.mozilla.org/mozilla-central/rev/ac8b78a93527

Comment 15

[Pascal Chevrel:pascalc (PTO until April 26)]

P2 crash fix, Kershaw, could your request an uplift to beta when you get a chance? Thanks

Comment 16

[Kershaw Chang [:kershaw]]

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.

Comment 17

[Pascal Chevrel:pascalc (PTO until April 26)]

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

Comment 18

[Andreea Pavel [:apavel]]

https://hg.mozilla.org/releases/mozilla-beta/rev/44b8aa9ccb46

Comment 19

[Daniel Bodea [:danibodea]]

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.

Comment 20

[Daniel Bodea [:danibodea]]

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!