392526 - Some callers of nsID::ToString use a mismatched allocator to free the string

Status: RESOLVED FIXED

(Core :: XPCOM, defect)

Description

[Mook (songbird dead account)]

Attachment: use PR_Free

http://bonsai.mozilla.org/cvsblame.cgi?file=/mozilla/xpcom/glue/nsID.cpp&rev=3.12&mark=127,129#120

nsID::ToString() allocates a string using PR_Malloc (in nspr4.dll)

http://bonsai.mozilla.org/cvsblame.cgi?file=/mozilla/xpcom/reflect/xptinfo/src/xptiManifest.cpp&rev=1.32&mark=99,114#91

xpti_InterfaceWriter frees it with nsCRT::free() which is PL_strfree (in plc4.dll)

... I crash because it's being freed from the wrong heap (being the wrong dll)

No, I have no idea how I managed to use the static CRT there.  But this code is definite not right.

Comment 1

[:Mook]

Attachment: kill evil bogus comment

Additional patch to remove a completely bogus comment, as pointed out by timeless over IRC.

Comment 2

[:Mook]

Comment on attachment 277029 [details] [diff] [review]
use PR_Free

Fixes a crash when (for some yet to be determined reason that is my fault) nspr and plc4 are linked against the static CRT.  One line change, low risk.

Sorry for the botch making this two patches :(

Comment 3

[Boris Zbarsky [:bzbarsky]]

So...  These look fine, but we have three problems here:

1)  The function doesn't document what allocator the caller should use
2)  The function uses an allocator which is not necessarily compatible with
    XPCOM strings (though in the common case it is).  As a result, Adopt() of
    this string is wrong.
3)  As a result, we have a number of confused consumers.

To be more precise, I see issues in the following consumers:

* nsComponentManagerImpl::AddPendingCID (uses Adopt())
* nsComponentManagerImpl::CreateInstance (uses Adopt() 3 times)
* nsComponentManagerImpl::CreateInstanceByContractID (uses Adopt())
* nsComponentManagerImpl::GetService (uses Adopt() twice)
* nsComponentManagerImpl::IsServiceInstantiated (uses Adopt() twice)
* nsComponentManagerImpl::IsServiceInstantiatedByContractID (uses Adopt())
* nsComponentManagerImpl::GetServiceByContractID (uses Adopt())
* nsVariant.cpp (frees with nsMemory::Free)
* nsSupportsIDImpl::ToString (hands result to xpcom out param)
* InitUserID in toolkit/crashreporter/nsExceptionHandler.cpp (leaks)
* Py_nsIID::PyTypeMethod_getattr (frees with nsMemory::Free)
* Py_nsIID::PyTypeMethod_repr (frees with nsMemory::Free)
* Py_nsIID::PyTypeMethod_str (frees with nsMemory::Free)
* PyXPCOM_TypeObject::Py_repr (frees with nsMemory::Free)
* PyG_Base::QueryInterface (frees with some random allocator)
* IPC_OnMessageAvailable (frees with nsMemory::Free)
* nsXPCComponents_InterfacesByID::NewEnumerate (frees with nsMemory::Free)
* nsScriptSecurityManager::CanCreateWrapper (frees with nsCRT::free)
* nsScriptSecurityManager::CheckComponentPermissions (uses Adopt())
* nsScriptSecurityManager::CanCreateInstance (frees with nsCRT::free and leaks)
* nsScriptSecurityManager::CanGetService (frees with nsCRT::free and leaks)

I assumed that NS_Free is equivalent to PR_Free for this purpose; if it's not, we have some other consumers using NS_Free.

Benjamin, do we care about the inconsistentcy between PR_Alloc and nsMemory::Free that a lot of these consumers have?  I would think yes....

Comment 4

[Benjamin Smedberg]

Yes, although it's not an immediate concern. We may in the future make NS_Alloc forward directly to malloc() instead of going through NSPR first, and then these callers would all potentially crash.

Comment 5

[Boris Zbarsky [:bzbarsky]]

Hold on.  So we have right now the PR_* allocators, the NS_* allocators, and nsMemory::*.  You're saying we should treat them all as distinct, right?

In that case all the nsMemory callers above need fixing, as do the various NS_Free callers for this code.  And if that's the situation we're in, I would go ahead and change this to use nsMemory throughout to be compatible with all the other strings in our code...

Comment 6

[Benjamin Smedberg]

nsMemory::foo should be equivalent to NS_Alloc/Free (they should be inlined equivalents of eachother).

Comment 7

[Boris Zbarsky [:bzbarsky]]

Ah, indeed.  So which do we want to be using here?  PR_* or NS_*?

Comment 8

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 277029 [details] [diff] [review]
use PR_Free

Removing approval requests pending reviews

Comment 9

[(dormant account)]

I did a little dehydra codescan on my codebase and found a few more instances of this.
Here is my complete list. Note that my dehydra script fails to see all of the stuff that bz found because it's hidden under flags like #ifdef SHOW_DENIED_ON_SHUTDOWN, which aren't on in my build.

nsComponentManager.cpp:1506: GetClassObject() :struct nsID const &
nsComponentManager.cpp:1584: ContractIDToClassID() :struct nsID *
nsComponentManager.cpp:1614: CLSIDToContractID() :struct nsID const &
nsComponentManager.cpp:1638: AddPendingCID() :struct nsID const &
nsComponentManager.cpp:1730: CreateInstance() :struct nsID const &
nsComponentManager.cpp:2453: RegisterFactory() :struct nsID const &
nsComponentManager.cpp:2617: RegisterComponentCommon() :struct nsID const &
nsComponentManager.cpp:2818: UnregisterFactory() :struct nsID const &
nsNavBookmarks.cpp:249: Init() :struct nsID
nsNullPrincipal.cpp:106: Init() :struct nsID
nsScriptSecurityManager.cpp:2915: CanCreateInstance() :struct nsID const &
nsScriptSecurityManager.cpp:2946: CanGetService() :struct nsID const &
nsSupportsPrimitives.cpp:94: ToString() :struct nsID *
nsUConvModule.cpp:486: nsUConverterUnregSelf() :struct nsID
nsUCvMathModule.cpp:98: nsUConverterUnregSelf() :struct nsID
nsVariant.cpp:813: ToString() :struct nsID
xpccomponents.cpp:635: NewEnumerate() :struct nsID const *
xpccomponents.cpp:659: NewEnumerate() :struct nsID const *
xpcjsid.cpp:112: GetNumber() :struct nsID
xpcjsid.cpp:117: GetNumber() :struct nsID
xpcjsid.cpp:417: GetNumber() :struct nsID const *
xpcjsid.cpp:979: xpc_NewIDObject() :struct nsID const &
xpcwrappedjsclass.cpp:1736: DebugDump() :struct nsID
xpcwrappedjs.cpp:647: DebugDump() :struct nsID const &
xptiManifest.cpp:99: xpti_InterfaceWriter() :struct nsID const *

Comment 10

[Robert Kaiser]

I suspect that this problem is actually what happens to a lot of people who are currently seeing bug 394190 despite having a VC8 CRT redist installed.

Comment 11

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 277029 [details] [diff] [review]
use PR_Free

sr- per comments

Comment 12

[[e-mail]]

Comment on attachment 277029 [details] [diff] [review]
use PR_Free

this particular patch (the namesake of this bug) is obsolete, per bug 410250. leaving open for the comment tweak, though?

Comment 13

[:Mook]

Yay for having a RSS feed of commits

Comment 14

[[e-mail]]

I wonder whether any of cases from comment #9 remain or whether they have all been fixed by bug 410250 and/or bug 331165...

Comment 15

[Boris Zbarsky [:bzbarsky]]

I'm reopening this bug until we actually check that; the whole point is that just changing the allocator in the function without changing callers leads to (a different set of) broken callers...

At the very least the security manager callers are still a problem.

Comment 16

[jag (Peter Annema)]

http://mxr.mozilla.org/comm-central/search?string=ToString\(\)&regexp=on&case=on&find=&findi=&filter=\bToString&hitlimit=&tree=comm-central

A lot use PR_Free(), some use nsMemory::Free() directly, and some Adopt() into a string object, which uses nsMemory::Free().

Where possible I suggest we convert to use .Adopt(id.ToString()).

Comment 17

[David Zbarsky (:dzbarsky)]

Attachment: fixes callers to use NS_Free

I wasn't sure what was going on here:
extensions/python/xpcom/src/PyGBase.cpp
in PyG_Base::QueryInterface

324 #ifdef PYXPCOM_DEBUG_FULL
325         {
326                 char *sziid = iid.ToString();
327                 LogF("PyGatewayBase::QueryInterface: %s", sziid);
328                 Allocator::Free(sziid);
329         }
330 #endif

What is this Allocator?

Comment 18

[Boris Zbarsky [:bzbarsky]]

> +    key = entry->charset;                                               \

You added trailing whitespace on this line.  Please take it out?

Comment 19

[Boris Zbarsky [:bzbarsky]]

David, see comment 18.

Comment 20

[David Zbarsky (:dzbarsky)]

Attachment: fixes callers, no whitespace

Comment 21

[Boris Zbarsky [:bzbarsky]]

Pushed http://hg.mozilla.org/mozilla-central/rev/44fab2f1ab64