1349621 - Use of uninitialized memory [@ NS_GetFinalChannelURI]

Status: RESOLVED FIXED

(Core :: Networking, defect)

Description

[Tyson Smith [:tsmith]]

I'm not sure if core networking is the correct component but I'll start here.

Conditional jump or move depends on uninitialised value(s)
   at 0x11411A66: NS_GetFinalChannelURI(nsIChannel*, nsIURI**) (nsNetUtil.cpp:1847)
   by 0x1249F4AF: nsContentSecurityManager::CheckChannel(nsIChannel*) (nsContentSecurityManager.cpp:553)
   by 0x124AAB32: nsContentSecurityManager::doContentSecurityCheck(nsIChannel*, nsCOMPtr<nsIStreamListener>&) (nsContentSecurityManager.cpp:475)
   by 0x119858B4: nsExtProtocolChannel::AsyncOpen2(nsIStreamListener*) (nsExternalProtocolHandler.cpp:227)
   by 0x1197E76D: nsURILoader::OpenURI(nsIChannel*, unsigned int, nsIInterfaceRequestor*) (nsURILoader.cpp:857)
   by 0x130028E8: nsDocShell::DoChannelLoad(nsIChannel*, nsIURILoader*, bool) (nsDocShell.cpp:11519)
   by 0x13013F03: nsDocShell::DoURILoad(nsIURI*, nsIURI*, bool, nsIURI*, bool, unsigned int, nsIPrincipal*, nsIPrincipal*, char const*, nsAString const&, nsIInputStream*, nsIInputStream*, bool, nsIDocShell**, nsIRequest**, bool, bool, bool, nsAString const&, nsIURI*, unsigned int) [clone .part.380] (nsDocShell.cpp:11333)
   by 0x1301B2A5: nsDocShell::InternalLoad(nsIURI*, nsIURI*, bool, nsIURI*, unsigned int, nsIPrincipal*, nsIPrincipal*, unsigned int, nsAString const&, char const*, nsAString const&, nsIInputStream*, nsIInputStream*, unsigned int, nsISHEntry*, bool, nsAString const&, nsIDocShell*, nsIURI*, bool, nsIDocShell**, nsIRequest**) (nsDocShell.cpp:10792)
   by 0x1301FA88: nsDocShell::LoadURI(nsIURI*, nsIDocShellLoadInfo*, unsigned int, bool) [clone .part.452] (nsDocShell.cpp:1591)
   by 0x11CD2538: nsFrameLoader::ReallyStartLoadingInternal() (nsFrameLoader.cpp:873)
   by 0x11CD2622: nsFrameLoader::ReallyStartLoading() (nsFrameLoader.cpp:757)
   by 0x11CD272F: nsDocument::MaybeInitializeFinalizeFrameLoaders() (nsDocument.cpp:7196)
 Uninitialised value was created by a heap allocation
   at 0x4C2DB8F: malloc (in /usr/lib/valgrind/vgpreload_memcheck-amd64-linux.so)
   by 0x4056C4: moz_xmalloc (mozalloc.cpp:83)
   by 0x11988AC1: operator new (mozalloc.h:194)
   by 0x11988AC1: nsExternalProtocolHandler::NewChannel2(nsIURI*, nsILoadInfo*, nsIChannel**) (nsExternalProtocolHandler.cpp:554)
   by 0x11417F05: mozilla::net::nsIOService::NewChannelFromURIWithProxyFlagsInternal(nsIURI*, nsIURI*, unsigned int, nsILoadInfo*, nsIChannel**) (nsIOService.cpp:793)
   by 0x1141720B: NS_NewChannelInternal(nsIChannel**, nsIURI*, nsILoadInfo*, nsILoadGroup*, nsIInterfaceRequestor*, unsigned int, nsIIOService*) (nsNetUtil.cpp:177)
   by 0x130133EE: nsDocShell::DoURILoad(nsIURI*, nsIURI*, bool, nsIURI*, bool, unsigned int, nsIPrincipal*, nsIPrincipal*, char const*, nsAString const&, nsIInputStream*, nsIInputStream*, bool, nsIDocShell**, nsIRequest**, bool, bool, bool, nsAString const&, nsIURI*, unsigned int) [clone .part.380] (nsDocShell.cpp:11047)
   by 0x1301B2A5: nsDocShell::InternalLoad(nsIURI*, nsIURI*, bool, nsIURI*, unsigned int, nsIPrincipal*, nsIPrincipal*, unsigned int, nsAString const&, char const*, nsAString const&, nsIInputStream*, nsIInputStream*, unsigned int, nsISHEntry*, bool, nsAString const&, nsIDocShell*, nsIURI*, bool, nsIDocShell**, nsIRequest**) (nsDocShell.cpp:10792)
   by 0x1301FA88: nsDocShell::LoadURI(nsIURI*, nsIDocShellLoadInfo*, unsigned int, bool) [clone .part.452] (nsDocShell.cpp:1591)
   by 0x11CD2538: nsFrameLoader::ReallyStartLoadingInternal() (nsFrameLoader.cpp:873)
   by 0x11CD2622: nsFrameLoader::ReallyStartLoading() (nsFrameLoader.cpp:757)
   by 0x11CD272F: nsDocument::MaybeInitializeFinalizeFrameLoaders() (nsDocument.cpp:7196)
   by 0x11CD28B0: nsDocument::EndUpdate(unsigned int) (nsDocument.cpp:5030)

Comment 1

[Patrick McManus [:mcmanus]]

do you have an about:buildconfig cset so we're looking at the same code? (always a useful thing to include with a stacktrace..)

Comment 2

[Tyson Smith [:tsmith]]

Attachment: build_config.txt

Built from https://hg.mozilla.org/mozilla-central/rev/e03e0c60462c775c7558a1dc9d5cf2076c3cd1f9

See attached build_config.txt for full details.

Comment 3

[Tyson Smith [:tsmith]]

Attachment: test_case.html

Comment 4

[Andrew McCreight [:mccr8]]

It looks like nsExternalProtocolHandler::mLoadFlags is not intialized in the ctor. Maybe that's it.

Comment 5

[Andrew McCreight [:mccr8]]

I'm going to mark this sec-moderate. If we end up bypassing a security check by returning the wrong URI, that sounds bad, but I don't know how much that matters for the mailto: protocol. (Tyson said it didn't work with other protocols, but presumably the user could register other handlers.)

Comment 6

[Patrick McManus [:mcmanus]]

(In reply to Andrew McCreight [:mccr8] from comment #4)
> It looks like nsExternalProtocolHandler::mLoadFlags is not intialized in the
> ctor. Maybe that's it.

worth trying - some kind of impressive inlining tho. the naive complaint is about something on the stack clearly initialized being &'d with a constant.

Comment 7

[Patrick McManus [:mcmanus]]

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

Comment 8

[Patrick McManus [:mcmanus]]

tyson - can you very the cset in comment 7? my machine wasn't up to the challenge of your testcase+valgrind :)

Comment 9

[Tyson Smith [:tsmith]]

(In reply to Patrick McManus [:mcmanus] from comment #8)
> tyson - can you very the cset in comment 7? my machine wasn't up to the
> challenge of your testcase+valgrind :)

Verified, the patch removes the issue.

Comment 10

[Patrick McManus [:mcmanus]]

bizarrely honza patched the same long standing issue in the last 24 hours. I won't mark it as dup just because this is a sec bug - but this is really a dup of 1349987

Comment 11

[Ryan VanderMeulen [:RyanVM]]

Maybe worth landing the testcase at some point still.