731184 - OOM Crash [@ InMemoryDataSource::~InMemoryDataSource] due to unhandled alloc failure of InMemoryDataSource::Init()

Status: RESOLVED INCOMPLETE

(Core Graveyard :: RDF, defect)

Description

[Christian Holler (:decoder)]

Tested on m-c revision 66e4d53697c2: The method InMemoryDataSource::Init() may fail to allocate but this doesn't seem to be handled properly somewhere as allocation failure will cause the following crash:


Program received signal SIGSEGV, Segmentation fault.
0x00002aaaacfb7e05 in InMemoryDataSource::~InMemoryDataSource (this=0xdc8530, __in_chrg=<optimized out>) at /srv/repos/browser/mozilla-central/rdf/base/src/nsInMemoryDataSource.cpp:956
956         PR_LOG(gLog, PR_LOG_NOTICE,
#0  0x00002aaaacfb7e05 in InMemoryDataSource::~InMemoryDataSource (this=0xdc8530, __in_chrg=<optimized out>) at /srv/repos/browser/mozilla-central/rdf/base/src/nsInMemoryDataSource.cpp:956
#1  0x00002aaaacfb7e50 in InMemoryDataSource::~InMemoryDataSource (this=0xdc8530, __in_chrg=<optimized out>) at /srv/repos/browser/mozilla-central/rdf/base/src/nsInMemoryDataSource.cpp:959
#2  0x00002aaaacfb80b0 in InMemoryDataSource::Internal::Release (this=0xdc8620) at /srv/repos/browser/mozilla-central/rdf/base/src/nsInMemoryDataSource.cpp:991
#3  0x00002aaaacfb8de5 in NS_NewRDFInMemoryDataSource (aOuter=<optimized out>, aIID=..., aResult=0x7fffffffade0) at /srv/repos/browser/mozilla-central/rdf/base/src/nsInMemoryDataSource.cpp:880
#4  0x00002aaaad282bea in CreateInstanceByContractID (aResult=0x7fffffffade0, aIID=..., aDelegate=0x0, aContractID=0x2aaaada36ca4 "@mozilla.org/rdf/datasource;1?name=in-memory-datasource", this=<optimized out>) at /srv/repos/browser/mozilla-central/xpcom/components/nsComponentManager.cpp:1064
#5  nsComponentManagerImpl::CreateInstanceByContractID (this=<optimized out>, aContractID=<optimized out>, aDelegate=0x0, aIID=..., aResult=0x7fffffffade0) at /srv/repos/browser/mozilla-central/xpcom/components/nsComponentManager.cpp:1013
#6  0x00002aaaad244e35 in nsCreateInstanceByContractID::operator() (this=0x7fffffffad48, aIID=<optimized out>, aInstancePtr=0x7fffffffade0) at /srv/repos/browser/mozilla-central/objdir-ff-gcc64dbg/xpcom/build/nsComponentManagerUtils.cpp:210
#7  0x00002aaaacfd07e3 in assign_from_helper (aIID=..., helper=..., this=0xdc6398) at ../../../dist/include/nsCOMPtr.h:1232


The backtrace of the failing allocation is as follows:

#0 /srv/repos/browser/mozilla-central/objdir-ff-gcc64dbg/dist/bin/libmozalloc.so(moz_malloc+0x5f) [0x2aaaaab2415c] (aab2415c)
#1 PL_DHashTableInit at objdir-ff-gcc64dbg/xpcom/build/pldhash.cpp:270
#2 InMemoryDataSource::Init() at rdf/base/src/nsInMemoryDataSource.cpp:912
#3 NS_NewRDFInMemoryDataSource(nsISupports*, nsID const&, void**) at rdf/base/src/nsInMemoryDataSource.cpp:874
#4 nsComponentManagerImpl::CreateInstanceByContractID(char const*, nsISupports*, nsID const&, void**) at xpcom/components/nsComponentManager.cpp:1065
#5 nsCreateInstanceByContractID::operator()(nsID const&, void**) const at objdir-ff-gcc64dbg/xpcom/build/nsComponentManagerUtils.cpp:211
#6 nsCOMPtr<nsIRDFDataSource>::assign_from_helper(nsCOMPtr_helper const&, nsID const&) at objdir-ff-gcc64dbg/dist/include/nsCOMPtr.h:1232
#7 nsWindowDataSourceConstructor at xpfe/components/windowds/nsWindowDataSource.cpp:576
#8 nsComponentManagerImpl::CreateInstanceByContractID(char const*, nsISupports*, nsID const&, void**) at xpcom/components/nsComponentManager.cpp:1065
#9 nsComponentManagerImpl::GetServiceByContractID(char const*, nsID const&, void**) at xpcom/components/nsComponentManager.cpp:1466

Comment 1

[Benjamin Smedberg]

I don't understand what allocation is failing incorrectly, can you help explain?

Both of the calls to PL_DHashTableInit in InMemoryDataSource::Init are properly checked and if they fail we clear out .ops.

Both of the calls in the destructor check .ops before doing anything.

But the stack given for the crash site also doesn't appear to be using the allocation you mention: it appears to be on the PR_LOG line. Is it possible that the line "gLog = PR_NewLogModule("InMemoryDataSource");" is failing? If so, we use that pattern unchecked throughout the codebasse and I don't think we should change it.

Comment 2

[Christian Holler (:decoder)]

I double checked this, and the allocation trace (the second trace of comment 0) is the trace emitted for the allocation failure. My patch emits this trace when it artificially causes an OOM for a certain call to a fallible allocator (e.g. moz_malloc). However, I checked in GDB what caused the crash, and it's indeed gLog being NULL:

(gdb) p gLog
$5 = (PRLogModuleInfo *) 0x0

Maybe this isn't visible from the allocation trace because this frame is optimized away.

My problem with accepting oom crashes that don't use the intended abort functions to signal a controlled crash will cause automated tools like mine to detect these as errors.

I would have proposed to use something like | if (UNLIKELY(!gLog)) someAbort() | but this is NSPR code anyway so I guess it cannot be changed that easily.

Comment 3

[Benjamin Smedberg]

Yes, we cannot change NSPR. We could wrap NSPR with an infallible version of PR_NewLogModule. But since most of our log modules are allocated during startup or statically i.e. static PRLogModuleInfo *gBMPLog = PR_NewLogModule("BMPDecoder"); We decided in the past that it really doesn't matter. Can you skip PR_NewLogModule in your OOM checker?

Comment 4

[Christian Holler (:decoder)]

(In reply to Benjamin Smedberg  [:bsmedberg] from comment #3)
> Can you skip PR_NewLogModule in your OOM checker?

Not using this function name, because it doesn't appear anywhere in the trace. But I could probably check if the failing source line is PR_LOG, or exclude ~InMemoryDataSource if that's always the function triggering the crash. I think I'll check how this behaves on other Mochitests (this is just a single run).