110263 - static const NS_NAMED_LITERAL_STRING is not thread safe

Status: RESOLVED FIXED

(Core :: XPConnect, defect)

Description

[David Bradley]

There are two instances of static const declarations of this that are not thread
safe, at least under Windows. VC++ generates code that tests a flag for creation
that is subject to a race condition. The window of vulnerability is quite small,
but does exist.

http://lxr.mozilla.org/seamonkey/source/js/src/xpconnect/src/xpcconvert.cpp#554

Simple solution is to remove static, but that would incure two allocations for
the strings. This occurs in conversions of DOM Strings which occurs fairly
often. Using a lock seems rather heavy handed as well.

Comment 1

[John Bandhauer]

I really doubt you can even construct a test that will trigger any problem here.
I'd let it ride.

Comment 2

[David Bradley]

You'd have to have two threads converting undefined/null to a DOM string all for
the first time. Given I generally don't buy lottery tickets and there's not a
simple inexpensive fix, futuring.

Comment 3

[Darin Fisher]

Maybe the code has changed, but the trivial fix now is to just use string
literals, no?

  const PRUnichar sEmptyString[] = {'\0'};
  const PRUnichar sVoidString[] = {'u','n','d','e','f','i','n','e','d'.'\0'};

Comment 4

[David Bradley]

Attachment: Use array initialization

Good point. This patch uses the formentioned array initalization which
eliminates the very slight threading issue.

Comment 5

[Darin Fisher]

Comment on attachment 157013 [details] [diff] [review]
Use array initialization

r=darin

Comment 6

[David Bradley]

Comment on attachment 157013 [details] [diff] [review]
Use array initialization

JST want to SR this? Also will have some tiny code size reduction.

Comment 7

[Johnny Stenback (:jst)]

Comment on attachment 157013 [details] [diff] [review]
Use array initialization

+	     static const PRUnichar EMPTY_STRING[] = { 0 };
+	     static const PRUnichar VOID_STRING[] = { 'u', 'n', 'd', 'e', 'f',
'i', 'n', 'e', 'd', 0 };

I'd do s/0/'\0'/ to make it clearer that we're dealing with null terminators
here.

sr=jst either way.

Comment 8

[Peter Van der Beken [:peterv]]

David: will you check this in or should I?

Comment 9

[David Bradley]

I was going to, I was hoping to have my cvs ssh key thingy working, but I 
haven't heard back after sending in the key. So if you have some time, feel 
free.

Comment 10

[Peter Van der Beken [:peterv]]

I checked this in with one small change, I changed

                    length = sizeof(VOID_STRING) / sizeof(PRUnichar);

to

                    length = NS_ARRAY_LENGTH(VOID_STRING) - 1;

(we don't want to count the terminating null-character in the length).