Closed Bug 392526 Opened 12 years ago Closed 10 years ago

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

Categories

(Core :: XPCOM, defect)

defect
Not set

Tracking

()

RESOLVED FIXED

People

(Reporter: mook, Assigned: dzbarsky)

References

()

Details

(Keywords: student-project, Whiteboard: [good first bug])

Attachments

(2 files, 2 obsolete files)

Attached patch use PR_Free (obsolete) — Splinter Review
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.
Attachment #277029 - Flags: review?(timeless)
Attachment #277029 - Flags: review?(timeless) → review+
Additional patch to remove a completely bogus comment, as pointed out by timeless over IRC.
Attachment #277053 - Flags: review?(timeless)
Attachment #277053 - Flags: review?(timeless) → review+
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 :(
Attachment #277029 - Flags: superreview?(bzbarsky)
Attachment #277029 - Flags: approval1.9?
Attachment #277053 - Flags: superreview?(bzbarsky)
Attachment #277053 - Flags: approval1.9?
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....
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.
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...
nsMemory::foo should be equivalent to NS_Alloc/Free (they should be inlined equivalents of eachother).
Ah, indeed.  So which do we want to be using here?  PR_* or NS_*?

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

Removing approval requests pending reviews
Attachment #277029 - Flags: approval1.9?
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 *
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 on attachment 277029 [details] [diff] [review]
use PR_Free

sr- per comments
Attachment #277029 - Flags: superreview?(bzbarsky) → superreview-
Attachment #277053 - Flags: superreview?(bzbarsky) → superreview-
Depends on: 410250
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?
Attachment #277029 - Attachment is obsolete: true
Assignee: nobody → mook
Summary: xpti_InterfaceWriter uses wrong free, crash when using static CRT → remove bogus comment about PR_Malloc/nsCRT::free from nsID::ToString
Component: XPCOM Registry → XPCOM
Yay for having a RSS feed of commits
Status: NEW → RESOLVED
Closed: 11 years ago
Resolution: --- → DUPLICATE
Duplicate of bug: 331165
I wonder whether any of cases from comment #9 remain or whether they have all been fixed by bug 410250 and/or bug 331165...
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.
Status: RESOLVED → REOPENED
Flags: blocking1.9.2?
Resolution: DUPLICATE → ---
Summary: remove bogus comment about PR_Malloc/nsCRT::free from nsID::ToString → Some callers of nsID::ToString use a mismatched allocator to free the string
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()).
Whiteboard: [good first bug]
Flags: blocking1.9.2? → blocking1.9.2-
Attached patch fixes callers to use NS_Free (obsolete) — Splinter Review
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?
Attachment #390307 - Flags: superreview?(benjamin)
Attachment #390307 - Flags: review?(benjamin)
> +    key = entry->charset;                                               \

You added trailing whitespace on this line.  Please take it out?
Attachment #390307 - Attachment is obsolete: true
Attachment #390484 - Flags: superreview?(benjamin)
Attachment #390484 - Flags: review?(benjamin)
Attachment #390307 - Flags: superreview?(benjamin)
Attachment #390307 - Flags: review?(benjamin)
Attachment #390484 - Flags: superreview?(benjamin)
Attachment #390484 - Flags: review?(benjamin)
Attachment #390484 - Flags: review+
Assignee: mook → dzbarsky
Pushed http://hg.mozilla.org/mozilla-central/rev/44fab2f1ab64
Status: REOPENED → RESOLVED
Closed: 11 years ago10 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.