nsThreadManager::GetCurrentThread() accesses TLS before initialization

ASSIGNED
Assigned to

Status

()

Core
XPCOM
ASSIGNED
3 years ago
4 months ago

People

(Reporter: jchen, Assigned: jchen)

Tracking

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(2 attachments)

(Assignee)

Description

3 years ago
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
(Assignee)

Comment 1

3 years ago
Created attachment 8543301 [details] [diff] [review]
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*.
Attachment #8543301 - Flags: review?(nfroyd)

Comment 2

3 years ago
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.
(Assignee)

Comment 3

3 years ago
(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

3 years ago
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.
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?
Flags: needinfo?(nchen)
(Assignee)

Comment 6

3 years ago
Created attachment 8544168 [details] [diff] [review]
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.
Attachment #8544168 - Flags: review?(nfroyd)
(Assignee)

Updated

3 years ago
Flags: needinfo?(nchen)
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.
Attachment #8543301 - Flags: review?(nfroyd)
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"
Attachment #8544168 - Flags: review?(nfroyd)
Attachment #8544168 - Flags: review?(benjamin)
Attachment #8544168 - Flags: review+

Comment 9

3 years ago
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.
Attachment #8544168 - Flags: review?(benjamin) → review+
You need to log in before you can comment on or make changes to this bug.