1331295 - crash near null [@nsNullPrincipal::CreateWithInheritedAttributes]

Status: RESOLVED FIXED

(Core :: Security: CAPS, defect)

Description

[Tyson Smith [:tsmith]]

Attachment: test_case.html

==24308==ERROR: AddressSanitizer: SEGV on unknown address 0x000000000000 (pc 0x7f0bbda83399 bp 0x7ffc304f2510 sp 0x7ffc304f2460 T0)
    #0 0x7f0bbda83398 in nsNullPrincipal::CreateWithInheritedAttributes(nsIPrincipal*) /home/worker/workspace/build/src/caps/nsNullPrincipal.cpp:43:32
    #1 0x7f0bc3858e5f in nsDocShell::CreateAboutBlankContentViewer(nsIPrincipal*, nsIURI*, bool, bool) /home/worker/workspace/build/src/docshell/base/nsDocShell.cpp:8049:19
    #2 0x7f0bc37f7a59 in nsDocShell::EnsureContentViewer() /home/worker/workspace/build/src/docshell/base/nsDocShell.cpp:7941:17
    #3 0x7f0bc37f408c in nsDocShell::GetInterface(nsID const&, void**) /home/worker/workspace/build/src/docshell/base/nsDocShell.cpp:971:14
    #4 0x7f0bbbfa2142 in nsGetInterface::operator()(nsID const&, void**) const /home/worker/workspace/build/src/xpcom/glue/nsIInterfaceRequestorUtils.cpp:18:16
    #5 0x7f0bbbf93f15 in nsCOMPtr_base::assign_from_helper(nsCOMPtr_helper const&, nsID const&) /home/worker/workspace/build/src/xpcom/glue/nsCOMPtr.cpp:117:7
    #6 0x7f0bbd9d934e in nsCOMPtr /home/worker/workspace/build/src/obj-firefox/dist/include/nsCOMPtr.h:574:5
    #7 0x7f0bbd9d934e in nsDocLoader::DocLoaderIsEmpty(bool) /home/worker/workspace/build/src/uriloader/base/nsDocLoader.cpp:667
    #8 0x7f0bbd9dba34 in nsDocLoader::OnStopRequest(nsIRequest*, nsISupports*, nsresult) /home/worker/workspace/build/src/uriloader/base/nsDocLoader.cpp:612:5
    #9 0x7f0bbd9dc5ec in non-virtual thunk to nsDocLoader::OnStopRequest(nsIRequest*, nsISupports*, nsresult) /home/worker/workspace/build/src/uriloader/base/nsDocLoader.cpp:468:14
    #10 0x7f0bbc1187ab in mozilla::net::nsLoadGroup::RemoveRequest(nsIRequest*, nsISupports*, nsresult) /home/worker/workspace/build/src/netwerk/base/nsLoadGroup.cpp:633:18
    #11 0x7f0bbc9ddf26 in mozilla::net::nsHttpChannel::OnStopRequest(nsIRequest*, nsISupports*, nsresult) /home/worker/workspace/build/src/netwerk/protocol/http/nsHttpChannel.cpp:6877:9
    #12 0x7f0bbc1119d1 in nsInputStreamPump::OnStateStop() /home/worker/workspace/build/src/netwerk/base/nsInputStreamPump.cpp:714:9
    #13 0x7f0bbc10f971 in nsInputStreamPump::OnInputStreamReady(nsIAsyncInputStream*) /home/worker/workspace/build/src/netwerk/base/nsInputStreamPump.cpp:433:25
    #14 0x7f0bbbee6dad in nsInputStreamReadyEvent::Run() /home/worker/workspace/build/src/xpcom/io/nsStreamUtils.cpp:95:9
    #15 0x7f0bbbf3042b in nsThread::ProcessNextEvent(bool, bool*) /home/worker/workspace/build/src/xpcom/threads/nsThread.cpp:1240:7
    #16 0x7f0bbbfb42fc in NS_ProcessNextEvent(nsIThread*, bool) /home/worker/workspace/build/src/xpcom/glue/nsThreadUtils.cpp:390:10
    #17 0x7f0bbcd6c2df in mozilla::ipc::MessagePump::Run(base::MessagePump::Delegate*) /home/worker/workspace/build/src/ipc/glue/MessagePump.cpp:96:21
    #18 0x7f0bbccda2a8 in RunInternal /home/worker/workspace/build/src/ipc/chromium/src/base/message_loop.cc:238:3
    #19 0x7f0bbccda2a8 in RunHandler /home/worker/workspace/build/src/ipc/chromium/src/base/message_loop.cc:231
    #20 0x7f0bbccda2a8 in MessageLoop::Run() /home/worker/workspace/build/src/ipc/chromium/src/base/message_loop.cc:211
    #21 0x7f0bc2181e2f in nsBaseAppShell::Run() /home/worker/workspace/build/src/widget/nsBaseAppShell.cpp:156:3
    #22 0x7f0bc42173f1 in nsAppStartup::Run() /home/worker/workspace/build/src/toolkit/components/startup/nsAppStartup.cpp:283:19
    #23 0x7f0bc43ae04c in XREMain::XRE_mainRun() /home/worker/workspace/build/src/toolkit/xre/nsAppRunner.cpp:4494:10
    #24 0x7f0bc43af51e in XREMain::XRE_main(int, char**, mozilla::XREAppData const&) /home/worker/workspace/build/src/toolkit/xre/nsAppRunner.cpp:4623:8
    #25 0x7f0bc43b041c in XRE_main /home/worker/workspace/build/src/toolkit/xre/nsAppRunner.cpp:4714:16
    #26 0x4df90c in do_main /home/worker/workspace/build/src/browser/app/nsBrowserApp.cpp:319:10
    #27 0x4df90c in main /home/worker/workspace/build/src/browser/app/nsBrowserApp.cpp:452
    #28 0x7f0bd72a782f in __libc_start_main /build/glibc-t3gR2i/glibc-2.23/csu/../csu/libc-start.c:291
    #29 0x41ba38 in _start (/home/user/workspace/browsers/firefox_cnt/firefox+0x41ba38)

Comment 1

[Marcos Caceres [:marcosc]]

Confirmed, definitely crashes in 54.0a1 (2017-02-13) (64-bit)

Comment 2

[Ethan Tseng [:ethan]]

Hi Baku, could you help to look into this bug?

Comment 3

[Andrea Marchesini [:baku]]

Attachment: null.patch

It seems to me that we could have a null principal here (where null means a null pointer).

Comment 4

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 8837057 [details] [diff] [review]
null.patch

So we used to create null principal here before bug 1022229 (and not requiring aPrincipal to be non-null).
I think the current setup is just a bit broken.
We should still create null principal even if there isn't aPrincipal, and inherit OA from docshell?

Comment 5

[Andrea Marchesini [:baku]]

Attachment: null.patch

Comment 6

[Olli Pettay [:smaug][[e-mail]]]

Please add a testcase too. I guess the attached testcase as a crashtest would work.

Comment 7

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/5b38cc764e90
nsNullPrincipal should be created using OriginAttributes of the docShell in case the principal is null, r=smaug

Comment 8

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/5b38cc764e90

Comment 9

[Ryan VanderMeulen [:RyanVM]]

Please request Aurora/Beta approval on this when you get a chance.

Comment 10

[Andrea Marchesini [:baku]]

Comment on attachment 8837147 [details] [diff] [review]
null.patch

Approval Request Comment
[Feature/Bug causing the regression]: bug 1022229
[User impact if declined]: a crash can occur
[Is this code covered by automated tests?]: there is a crashtest included in the patch
[Has the fix been verified in Nightly?]: not yet 
[Needs manual test from QE? If yes, steps to reproduce]: no
[List of other uplifts needed for the feature/fix]: none
[Is the change risky?]: no
[Why is the change risky/not risky?]: We check if the principal is null, and in case we use a different CTOR of a null principal.
[String changes made/needed]: none

Comment 11

[Liz Henry (:lizzard Please n-i to RyanVM, jcristau, or pascal)]

Comment on attachment 8837147 [details] [diff] [review]
null.patch

Should fix some crashes, let's take it for beta 8. 
baku do you want to uplift the tests also?

Comment 12

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/1f51ffd2298e

Comment 13

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-beta/rev/0c2cd04d9813

Comment 14

[Andrea Marchesini [:baku]]

What landed contains the test. The crashtast is already in m-i/m-a/m-b.

Comment 15

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-esr52/rev/0c2cd04d9813

Comment 16

[Andrew Halberstadt [:ahal]]

Andrea, how important is the window.reload() at the end of this test? Is there a way to avoid that without making the test useless?

The reftest harness receives an event when the page loads, and that reload is causing us to do a bunch of stuff twice. It's preventing us from landing bug 1372565 and may be messing up some other state. Thanks!

Comment 17

[Andrea Marchesini [:baku]]

I think you can remove that window.reload() yes. That that is mainly copy-and-paste of the original crash test case.