Closed Bug 979278 Opened 6 years ago Closed 5 years ago

TSan: data race nsprpub/pr/src/threads/prtpd.c:103 PR_NewThreadPrivateIndex


(NSPR :: NSPR, defect, P1)



(Not tracked)



(Reporter: decoder, Assigned: wtc)


(Blocks 1 open bug)


(Whiteboard: [tsan])


(2 files)

The attached logfile shows a thread/data race (mozilla-central revision 626d99c084cb) detected by TSan (ThreadSanitizer).

Typically, races reported by TSan are not false positives, but it is possible that the race is benign. Even in this case though, we should try to come up with a fix unless this would cause inacceptable performance issues. Also note that seemingly benign races can possibly be harmful (also depending on the compiler and the architecture) [1].

If the bug cannot be fixed, then this bug should be used to either make a compile-time annotation for blacklisting or add an entry to the runtime blacklist.

The problematic code is the read of _pr_tpd_highwater on line 146:

137 PR_IMPLEMENT(PRStatus) PR_SetThreadPrivate(PRUintn index, void *priv)
138 {
139     PRThread *self = PR_GetCurrentThread();
141     /*
142     ** The index being set might not have a sufficient vector in this
143     ** thread. But if the index has been allocated, it's okay to go
144     ** ahead and extend this one now.
145     */
146     if ((index >= _PR_TPD_LIMIT) || (index >= _pr_tpd_highwater))
147     {
148         PR_SetError(PR_TPD_RANGE_ERROR, 0);
149         return PR_FAILURE;
150     }

_pr_tpd_highwater is maintained by PR_ATOMIC_INCREMENT. It should
not be read directly.

I think we should remove the index >= _pr_tpd_highwater check on
line 146. The comment on lines 142-144 can be deleted.
Severity: critical → normal
Priority: -- → P1
Target Milestone: --- → 4.10.5
Version: 3.0 → other
Christian, could you review and test this patch? Thanks.

The fix is to only access the _pr_tpd_highwater variable
using NSPR atomic functions. This means we can't do a
check like
    index >= _pr_tpd_highwater

POSIX Threads also allows skipping this test to improve

The equivalent error for pthread_setspecific is EINVAL,
"The key value is invalid.". Note that the man page uses
"may" as opposed to "shall" for that error. Also the Rationale
section says:

    Performance and ease-of-use of pthread_getspecific()
    are critical for functions that rely on maintaining
    state in thread-specific data. Since no errors are
    required to be detected by it, and since the only
    error that could be detected is the use of an invalid
    key, the function to pthread_getspecific() has been
    designed to favor speed and simplicity over error
Attachment #8395305 - Flags: review?(choller)
Comment on attachment 8395305 [details] [diff] [review]
Only access _pr_tpd_highwater using atomic functions

Review of attachment 8395305 [details] [diff] [review]:

I'm not sure if I can review this and I'd prefer to have it reviewed by someone who is actually familiar with NSPR code.

Joshua, can you review this, since you reviewed the last TSan NSPR patch? Thanks.
Attachment #8395305 - Flags: review?(choller) → review?(Pidgeot18)
Comment on attachment 8395305 [details] [diff] [review]
Only access _pr_tpd_highwater using atomic functions

Review of attachment 8395305 [details] [diff] [review]:

Sorry for the delay in reviewing this patch, but "thread-safety" and "NSPR" in my mind equate to "scary patch, takes forever to review" and hence kept getting put off until I could find the time to review it.

I also had to take the time to convince myself that the deletion of the check was safe, in that while its behavior may not notionally be well-defined, its actual runtime behavior should still avoid causing problems in the first place.
Attachment #8395305 - Flags: review?(Pidgeot18) → review+
Comment on attachment 8395305 [details] [diff] [review]
Only access _pr_tpd_highwater using atomic functions

Joshua: thanks for the careful review!

Patch checked in:
Attachment #8395305 - Flags: checked-in+
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: 4.10.5 → 4.10.7
I updated NSPR on inbound to take this fix:
You need to log in before you can comment on or make changes to this bug.