Closed Bug 414997 Opened 12 years ago Closed 12 years ago

PR_NewThreadPrivateIndex contract is violated by PR_CreateThread impls

Categories

(NSPR :: NSPR, defect, major)

All
Linux
defect
Not set
major

Tracking

(Not tracked)

RESOLVED FIXED

People

(Reporter: timeless, Assigned: wtc)

References

()

Details

(Whiteboard: blocking-beta-3)

Attachments

(3 files, 3 obsolete files)

Attached file testcase (obsolete) —
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?
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
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
Attached patch Proposed patch (obsolete) — Splinter Review
This patch has been verified to work in bug 414745.
Attachment #300553 - Flags: superreview?(julien.pierre.boogz)
Attachment #300553 - Flags: review?(kengert)
Blocks: 414745
Comment on attachment 300553 [details] [diff] [review]
Proposed patch

Looks reasonable.
Attachment #300553 - Flags: superreview?(julien.pierre.boogz) → superreview+
Comment on attachment 300553 [details] [diff] [review]
Proposed patch

looks good to me, r=kengert
Attachment #300553 - Flags: review?(kengert) → review+
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)
Blocks: 411743
I guess timeless did not see the other patch before he attached his patch?
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
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)
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.
Attached file 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
Attached patch for beta 3Splinter Review
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+
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
Closed: 12 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
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.