1318398 - Thread-unsafe access to global variable pt_debug

Status: RESOLVED INCOMPLETE

(NSPR :: NSPR, defect)

Description

[Jed Davis [:jld] ⟨⏰|UTC-8⟩ ⟦he/him⟧]

NSPR's thread library, if compiled with DEBUG, tries to keep some stats in the global variable pt_debug.  However, it's updated with normal thread-unsafe C operations.  In some cases lost updates to debugging stats might not be a significant problem, but this isn't one of them: the values in question are all balanced pairs of counters (created/destroyed, acquired/released, etc.), so lost updates will affect whether they balance as expected.

Normally I'd suggest using atomic operations to update the counters, but NSPR supports atomic operations on platforms that don't have them natively by polyfilling with locks, so that would be a circular dependency and probably self-deadlock.  The counters could just be disabled completely on those platforms.

Alternately, given that this has been broken since 1998[*] as far as I can tell, and nobody's noticed, maybe we could just remove the code entirely.

This bug was found by Helgrind, so I don't have a test case that demonstrates it, but starting a bunch of threads that do nothing but lock/unlock different locks should do it.


[*]: https://hg.mozilla.org/projects/nspr/diff/81fc3c4cf2b3/pr/include/private/primpl.h

Comment 1

[Yaron Tausky [:ytausky]]

This is observable when starting Firefox with TSan.

Comment 2

[Kris Wright :KrisWright]

I'm not sure if this was fixed upstream in nspr, but it doesn't seem to be used by tsan right now either.