111068 - SyncXPCContextList's use of XPCContext::Mark is not threadsafe

Status: VERIFIED FIXED

(Core :: XPConnect, defect)

Description

[John Bandhauer]

While testing the threadsafety of the fix to bug 109218 I hit the assert in
XPCContext::SetCallingLangType. This was part of the fix to bug 103649.

It turns out that fix had a problem we didn't anticipate. The locking there
protects us from the creation and destruction of XPCContexts but it does not
lock out code running on other threads from using those contexts. This means
that overloading the mCallingLangType to include the marked bit is not safe
because that member is manipulated as code on other threads is run.

You can see this assert by running
mozilla\js\src\xpconnect\tests\js\old\threads.js in the xpcshell.

I have a patch that adds a new JSPackedBool to hold this flag. By reordering the
data members this member fits in without increasing the instance size.

Comment 1

[John Bandhauer]

Attachment: proposed fix

Comment 2

[David Bradley]

r=dbradley
Cleaner code as well.

Comment 3

[Brendan Eich [:brendan]]

Hmm, are smaller-than-int members independently updated on all architectures? 
That is, if I pack two uint16s adjacent in a struct, and thread T messes with
the first while thread U loads and stores the second -- or more generally,
threads use lock L1 to protect the first and L2 to protect the second, will the
loads and stores cohere?

/be

Comment 4

[John Bandhauer]

brendan: I don't know the answer to that (esp. in regard to *all* platforms) any
better than you do. I do know that on Win32 mCallingLangType uses up an entire
word. mSecurityManagerFlags (which is sharing a word with mMarked) is immutable
after object init. So, at least on Win32, this issue is moot. Do you think it is
worth promoting mMarked to be a JSBool just in case *some* platform exists that
will pack all three members into a word and not atomically manipulate them? I'm
OK either way. Our average live context count is not so large that we ought to
worry much about the added size.

Comment 5

[Wan-Teh Chang]

Brendan:

The answer to your question is no.  This problem is
described in the last two paragraphs on page 93 of
David Butenhof's "Programming with POSIX Threads".
However, he doesn't say what data type makes the
loads and stores cohere.

In NSPR, I use the libc data type sig_atomic_t for
this purpose.  It's the closest thing I can find.

Comment 6

[John Bandhauer]

->0.9.8

Comment 7

[David Bradley]

http://groups.google.com/groups?hl=en&selm=34FD6950.ECDC1AB5%40zko.dec.com&prev=/groups%3Fq%3D%2522Oh,%2Babsolutely.%2BSPARC,%2BMIPS,%2Band%2BAlpha%2522%26hl%3Den%26rnum%3D1%26selm%3D34FD6950.ECDC1AB5%2540zko.dec.com
seems to have a good conversation of the problem of word tearing. There appear
to be some systems out there that have instructions for only manipulating 64
bits at a time, specifically old Alpha AXP systems. MIPS and Sparc are mentioned
but I wasn't able to find out if they are limitted to 64 bits or 32 bits.

If this is something worth worry about, is this the only place that this will be
a problem? The one other place that came to mind was XPCNativeInterface, but
everything around mMemberCount seems pretty constant so it seems safe.

Comment 8

[John Bandhauer]

Reassign to dbradley. He can figure out whether to just check this in or roll
this up with bug 117559.

Comment 9

[David Bradley]

I think we need to get this in. It's a simple fix and gets us to a better state
quickly. The SyncXPCContextList patch in bug 117559 has value, in that it
reduces construction of XPCContext's and doesn't iterator JS Contexts as much,
but its a larger riskier patch.

Can we get an sr= for this?

If the word tearing issue is something we really want to worry about, we could
reorder the members such that the non-changing members are near the mark variable.

    nsresult mLastResult;
    nsresult mPendingResult;
    nsIXPCSecurityManager* mSecurityManager;
    nsIException* mException;
    PRUint16 mSecurityManagerFlags;
    LangType mCallingLangType;
    XPCJSRuntime* mRuntime;
    JSContext* mJSContext;
    JSPackedBool mMarked;

Comment 10

[John Bandhauer]

Comment on attachment 58615 [details] [diff] [review]
proposed fix

[setting the flag for existing r=dbradley]

The members are already reordered so that the marking flag is paired 
with an immutable member.

You should send email to request sr=

Comment 11

[Johnny Stenback (:jst)]

Comment on attachment 58615 [details] [diff] [review]
proposed fix

sr=jst

Comment 12

[David Bradley]

fix checked in.

Regarding the ordering, there are systems, Alpha was one example, that only
store/retrieve data in quadwords and the operation isn't atomic. Depending on
how the compiler decides to align mCallingLangType, mSecurityManagerFiles, and
mMarked there could be word tearing with manipulation of mCallingLangType and
mMarked. I just don't know if this is common among 64 bit risc systems or not.
If it is I think we have more than just this piece of code to worry about ;-)

Comment 13

[Brendan Eich [:brendan]]

dbradley: can you file bugs on any other hazards you've found, or know of off
the top of your head?  Thanks,

/be

Comment 14

[Phil Schwartau]

Marking Verified -