PR_NewThreadPrivateIndex contract is violated by PR_CreateThread impls

RESOLVED FIXED in 4.7

Status

--
major
RESOLVED FIXED
11 years ago
11 years ago

People

(Reporter: timeless, Assigned: wtc)

Tracking

other
All
Linux
Dependency tree / graph
Bug Flags:
blocking1.9 +

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: blocking-beta-3, URL)

Attachments

(3 attachments, 3 obsolete attachments)

(Reporter)

Description

11 years ago
Created attachment 300539 [details]
testcase

plasticmillion was complaining that xpconnect was caught violating a jsapi contract. I explained that xpconnect was at the mercy of one of xpcom or nspr.

I think I've seen this before, but I clearly didn't spend enough time researching it.

The nspr contract for a Thread Private Index (tpi) as I read it states:

> the function will be called when:
> the thread exits and the private data for the associated index is not NULL,
> new thread private data is set and the current private data is not NULL.

The keyword is *exits*. Nowhere in that statement is a mention of "when some random unrelated thread calls PR_JoinThread".

The preamble states:
> This index can be used with the PR_SetThreadPrivate() and
> PR_GetThreadPrivate() routines to save and retrieve data associated with
> the index for a thread.

PR_GetThreadPrivate states:
> A thread can only get access to its own thread-specific-data.

doppler:~/nspr/mozilla timeless$ gcc -I nsprpub/dist/include/nspr/ -Lnsprpub/dist/lib -lnspr4  ./testcase.c 2>/dev/null; ./a.out ld: Undefined symbols:
_PR_DestroyThread
main start: 0x500bb0
function_test: 0x501c50 @ 0x3024 [1]
death_check: 0x501c50 @ 0x3024
death_check: 0x501c50 @ 0xf0100e30
function_test: 0x501d30 @ 0x3028 [2]
death_check: 0x501d30 @ 0x3028
death_check: 0x501d30 @ 0xf0201e30
death_check: 0x500bb0 @ 0x3024
death_check: 0x500bb0 @ 0x3028
main done: 0x500bb0

As you can see from the last two death_checks, the main thread has gotten access to private data from two other threads.

This is fairly serious as it undermines a core tenet of xpconnect's design.

And yes, I'm aware it's been broken since presumably forever. I'm sorry. I believe this needs to be fixed.
Flags: blocking1.9?
(Reporter)

Comment 1

11 years ago
ok. I've checked the BeOS and Windows implementations, and they're both ok. this bug is only present in the pthreads impl.

BeOS is elegant:
http://bonsai.mozilla.org/cvsblame.cgi?file=/mozilla/nsprpub/pr/src/bthreads/btthread.c&rev=3.9&mark=284#270
Win is a bit wordy:
http://bonsai.mozilla.org/cvsblame.cgi?file=/mozilla/nsprpub/pr/src/threads/combined/pruthr.c&rev=3.36&mark=466#436
OS: All → Linux
(Assignee)

Comment 2

11 years ago
Thanks for the bug report.  This bug was reported to me just minutes earlier
in bug 414745.  I have a working patch.  NSPR is in code freeze for the NSPR
4.7 release now, so I probably can only check in the fix after next Tuesday.
Status: NEW → ASSIGNED
Target Milestone: --- → 4.7.1
Version: 4.7.3 → other
(Assignee)

Comment 3

11 years ago
Created attachment 300553 [details] [diff] [review]
Proposed patch

This patch has been verified to work in bug 414745.
Attachment #300553 - Flags: superreview?(julien.pierre.boogz)
Attachment #300553 - Flags: review?(kengert)
(Assignee)

Updated

11 years ago
Blocks: 414745

Comment 4

11 years ago
Comment on attachment 300553 [details] [diff] [review]
Proposed patch

Looks reasonable.
Attachment #300553 - Flags: superreview?(julien.pierre.boogz) → superreview+

Comment 5

11 years ago
Comment on attachment 300553 [details] [diff] [review]
Proposed patch

looks good to me, r=kengert
Attachment #300553 - Flags: review?(kengert) → review+
(Reporter)

Comment 6

11 years ago
Created attachment 300565 [details] [diff] [review]
destroy thread private data (everyone else does it!)

doppler:~/nspr/mozilla timeless$ cvs diff -up nsprpub/pr/src/pthreads/ptthread.c > 414997; ./a.out 
main start: 0x500bb0
function_test: 0x501c50 @ 0x3024 [1]
death_check: 0x501c50 @ 0x3024
death_check: 0x501c50 @ 0xf0100e30
death_check: 0x501e50 @ 0x3024
function_test: 0x501d30 @ 0x3028 [2]
death_check: 0x501d30 @ 0x3028
death_check: 0x501d30 @ 0xf0201e30
death_check: 0x501e50 @ 0x3028
main done: 0x500bb0

This isn't ideal, however, I'm not actually sure it's wrong. Essentially PR_GetCurrentThread is invalid (0x501e50 is not a threadid we know) when each thread dies, but it is being called from the right thread.

I need to check to see what the testcase actually prints on windows (and BeOS). It's possible that _PR_DestroyThreadPrivate needs to be changed to destroy the PR_GetCurrentThread slot last, but if that's the case, I think it's technically its own bug.
Attachment #300565 - Flags: review?(wtc)

Updated

11 years ago
Blocks: 411743

Comment 7

11 years ago
I guess timeless did not see the other patch before he attached his patch?

Comment 8

11 years ago
So do we want to do another NSPR drop today/tommorrow before FF3Beta3 ships?
Hm, the NSPR code freeze makes things tricky. Kai, can we get a mini branch with this patch for beta 3?
Flags: blocking1.9? → blocking1.9+
Whiteboard: blocking-beta-3
(Reporter)

Comment 10

11 years ago
Comment on attachment 300565 [details] [diff] [review]
destroy thread private data (everyone else does it!)

yes, sorry, i was not reading the bug by this point. merely checkpointing.
Attachment #300565 - Attachment is obsolete: true
Attachment #300565 - Flags: review?(wtc)
(Assignee)

Comment 11

11 years ago
Comment on attachment 300565 [details] [diff] [review]
destroy thread private data (everyone else does it!)

timeless: thank you for your investigation of this bug on
*all* platforms and your patch.  Your patch also works.
The only issue is that _PR_DestroyThreadPrivate(thred) will
be called twice, which is harmless because the first call
clears all the pointers after freeing the data they point to.
(Reporter)

Comment 12

11 years ago
Created attachment 300648 [details]
testcase

Here's a corrected testcase. _PR_DestroyThread was some nspr pthread internal function. 

this is nspr from gecko on windos w/ msvc just to show that PR_GetCurrentThread behaves correctly:
dbg-firefox-i686-pc-mingw32\dist\bin>..\test\test
main start: 008F3158
function_test: 008F79F8 @ 0040C004 [1]
death_check: 008F79F8 @ 0040C004
death_check: 008F79F8 @ 00A0FF48
death_check: 008F79F8 @ 0040C004
function_test: 008F7EB0 @ 0040C008 [2]
death_check: 008F7EB0 @ 0040C008
death_check: 008F7EB0 @ 00B0FF48
death_check: 008F7EB0 @ 0040C008
main done: 008F3158

as to my patch, i knew that it'd get called twice, but code inspection indicated that would be harmless. it was really late into my morning, I looked at the logic for the various call sites and decided i couldn't puzzle through a more correct patch.
Attachment #300539 - Attachment is obsolete: true

Comment 13

11 years ago
Created attachment 300653 [details] [diff] [review]
for beta 3
Attachment #300653 - Flags: approval1.9b3?
Comment on attachment 300653 [details] [diff] [review]
for beta 3

a=beltzner for this new patched NSPR minibranch
Attachment #300653 - Flags: approval1.9b3? → approval1.9b3+

Comment 15

11 years ago
During the NSPR/NSS meeting it was decided to take this fix on the NSPR trunk right now.
I checked it in. Changing target milestone to 4.7
Marking bug as fixed.

Checking in pr/src/pthreads/ptthread.c;
/cvsroot/mozilla/nsprpub/pr/src/pthreads/ptthread.c,v  <--  ptthread.c
new revision: 3.78; previous revision: 3.77
Status: ASSIGNED → RESOLVED
Last Resolved: 11 years ago
Resolution: --- → FIXED
Target Milestone: 4.7.1 → 4.7
Checking in client.mk;
/cvsroot/mozilla/client.mk,v  <--  client.mk
new revision: 1.366; previous revision: 1.365
done
(Assignee)

Comment 17

11 years ago
Created attachment 300708 [details] [diff] [review]
Proposed patch v1.1

I added one more comment.  No need to give Mozilla a new NSPR tag
just for this comment change.

Checking in ptthread.c;
/cvsroot/mozilla/nsprpub/pr/src/pthreads/ptthread.c,v  <--  ptthread.c
new revision: 3.79; previous revision: 3.78
done
Attachment #300553 - Attachment is obsolete: true
You need to log in before you can comment on or make changes to this bug.