1116871 - nsThreadManager::GetCurrentThread() accesses TLS before initialization

Status: NEW

(Core :: XPCOM, defect)

Description

[(inactive) Jim Chen [:jchen] [:darchons]]

nsThreadManager::GetCurrentThread() [1] uses mCurThreadIndex to access TLS before checking that initialization is done. If mCurThreadIndex is not initialized, it can potentially point to someone else's TLS slot, and end up treating someone else's data as a valid nsThread entry.

The only reason that this hasn't been a problem is because right now nsThreadManager is the first TLS consumer. However, Fennec has some preloading code that uses TLS before nsThreadManager and this will be a problem soon.

[1] http://mxr.mozilla.org/mozilla-central/source/xpcom/threads/nsThreadManager.cpp?rev=b71ccef9c674#306

Comment 1

[(inactive) Jim Chen [:jchen] [:darchons]]

Attachment: Don't use TLS in nsThreadManager before initialization (v1)

We shouldn't use nsThreadManager::mCurThreadIndex to access TLS before nsThreadManager is initialized. Otherwise, we could end up reading an existing TLS entry and erroneously treating the value as a nsThread*.

Comment 2

[Benjamin Smedberg]

This doesn't look right to me. It doesn't make sense to be calling nsThreadManager::GetCurrentThread before we've initialized XPCOM; I think the correct solution here is to MOZ_ASSERT(mMainThread) and deal with the callsites that are incorrect.

Comment 3

[(inactive) Jim Chen [:jchen] [:darchons]]

(In reply to Benjamin Smedberg  [:bsmedberg] from comment #2)
> This doesn't look right to me. It doesn't make sense to be calling
> nsThreadManager::GetCurrentThread before we've initialized XPCOM; I think
> the correct solution here is to MOZ_ASSERT(mMainThread) and deal with the
> callsites that are incorrect.

The profiler calls GetCurrentThread before we initialize XPCOM [1, 2]. I guess we can fix the profiler if we have a way to detect if nsThreadManager has been initialized before or not, but if we have to add that to nsThreadManager, might as well make GetCurrentThread callable before nsThreadManager is initialized (we already allow GetCurrentThread calls post-shutdown anyways).

[1] http://mxr.mozilla.org/mozilla-central/source/tools/profiler/platform.cpp?rev=f9d49449c02d#118
[2] http://mxr.mozilla.org/mozilla-central/source/tools/profiler/platform.h?rev=f9d49449c02d&force=1&mark=425-427#425

Comment 4

[Benjamin Smedberg]

It seems clearly like a bug for GetCurrentThread to be called while the thread manager is dead. I don't understand why the profiler needs the XPCOM thread (there aren't multiple threads active before XPCOM startup anyway, right?). And probably the after-shutdown case should be fixed as well.

It might be ok to take this patch as a stopgap, but I'd consider it technical debt, not solid design.

Comment 5

[Nathan Froyd [:froydnj]]

Why can't we just fix the profiler to start after XPCOM startup?  I suppose you lose whatever samples you might have taken prior to XPCOM startup, but presumably there's not *that* much happening in that space?

Comment 6

[(inactive) Jim Chen [:jchen] [:darchons]]

Attachment: Move profiler initialization to after nsThreadManager initialization (v1)

(In reply to Nathan Froyd [:froydnj] [:nfroyd] from comment #5)
> Why can't we just fix the profiler to start after XPCOM startup?  I suppose
> you lose whatever samples you might have taken prior to XPCOM startup, but
> presumably there's not *that* much happening in that space?

I guess it's just a bit more complicated because we have different ways of initializing the profiler/XPCOM combo depending on the application (e.g. firefox, xpcshell, plugin-container).

Here's the patch that initializes the profiler after nsThreadManager gets initializd. I think it covers all the cases.

Comment 7

[Nathan Froyd [:froydnj]]

Comment on attachment 8543301 [details] [diff] [review]
Don't use TLS in nsThreadManager before initialization (v1)

Review of attachment 8543301 [details] [diff] [review]:
-----------------------------------------------------------------

Going to assume the other patch supersedes this one.

Comment 8

[Nathan Froyd [:froydnj]]

Comment on attachment 8544168 [details] [diff] [review]
Move profiler initialization to after nsThreadManager initialization (v1)

Review of attachment 8544168 [details] [diff] [review]:
-----------------------------------------------------------------

This looks OK to me, but I know very little about the startup sequence; Benjamin should probably take a second look.

::: xpcom/build/XPCOMInit.cpp
@@ +545,5 @@
>    if (NS_WARN_IF(NS_FAILED(rv))) {
>      return rv;
>    }
>  
> +  // Iniialize profiler after nsThreadManager is initialized.

Nit: "initialize"

Comment 9

[Benjamin Smedberg]

Comment on attachment 8544168 [details] [diff] [review]
Move profiler initialization to after nsThreadManager initialization (v1)

There is a functional difference here: in child processes that don't use XPCOM (plugin and GMP processes), we won't ever init the profiler. I really don't know anything about the profiler, so I don't know whether that's ok or not.

If the profiler is supposed to work in non-XPCOM processes, perhaps it should just avoid calling into the thread manager.

But I'm going to mark r+ in any case, because I figure as the profiler guy you know whether this accomplishes what you care about.