Closed
Bug 414997
Opened 16 years ago
Closed 16 years ago
PR_NewThreadPrivateIndex contract is violated by PR_CreateThread impls
Categories
(NSPR :: NSPR, defect)
Tracking
(Not tracked)
RESOLVED
FIXED
4.7
People
(Reporter: timeless, Assigned: wtc)
References
()
Details
(Whiteboard: blocking-beta-3)
Attachments
(3 files, 3 obsolete files)
1.02 KB,
text/plain
|
Details | |
660 bytes,
patch
|
beltzner
:
approval1.9b3+
|
Details | Diff | Splinter Review |
1.84 KB,
patch
|
Details | Diff | Splinter Review |
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
Assignee | ||
Comment 2•16 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•16 years ago
|
||
This patch has been verified to work in bug 414745.
Attachment #300553 -
Flags: superreview?(julien.pierre.boogz)
Attachment #300553 -
Flags: review?(kengert)
Comment 4•16 years ago
|
||
Comment on attachment 300553 [details] [diff] [review] Proposed patch Looks reasonable.
Attachment #300553 -
Flags: superreview?(julien.pierre.boogz) → superreview+
Comment 5•16 years ago
|
||
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)
Comment 7•16 years ago
|
||
I guess timeless did not see the other patch before he attached his patch?
Comment 8•16 years ago
|
||
So do we want to do another NSPR drop today/tommorrow before FF3Beta3 ships?
Comment 9•16 years ago
|
||
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•16 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•16 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•16 years ago
|
||
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•16 years ago
|
||
Attachment #300653 -
Flags: approval1.9b3?
Comment 14•16 years ago
|
||
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•16 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
Closed: 16 years ago
Resolution: --- → FIXED
Target Milestone: 4.7.1 → 4.7
Comment 16•16 years ago
|
||
Checking in client.mk; /cvsroot/mozilla/client.mk,v <-- client.mk new revision: 1.366; previous revision: 1.365 done
Assignee | ||
Comment 17•16 years ago
|
||
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.
Description
•