17601 - Memory leak of thread private data in classic NSPR

Status: VERIFIED FIXED

(NSPR :: NSPR, defect, P3)

Description

[Wan-Teh Chang]

This bug is reported by Chris Houck.

In pruthr.c, _PR_NativeDestroyThread(),
the thread private data is not disposed
of and hence leaked.  This memory leak
is caught by Purify.

Going through the file, I spotted other
places where thread private data is not
disposed of.  We need to do a careful
code review.

The pthreads version does not seem to have
this problem.

Comment 1

[Wan-Teh Chang]

Attachment: The patch that Chris Houck submitted. Note that this is not the complete fix.

Comment 2

[Wan-Teh Chang]

I thoroughly reviewed our TPD code and fixed
other problems that I noticed.  I'm going to
post a patch with comments.

Comment 3

[Wan-Teh Chang]

Attachment: A patch for the memory leak and other problems in our TPD code.

Comment 4

[Wan-Teh Chang]

Comments on the patch (attachment id 2899):
1. primpl.h: the global variable _pr_tpd_destructors does
   not need to be exported from prtpd.c.
2. ptthread.c: removed unused field 'highwater' from pt_book.
   Removed commented out code.  _pt_thread_death now needs to
   free thred->privateData because _PR_DestroyThreadPrivate no
   longer does that.
3. prcthr.c: _PR_CleanupThread should call _PR_DestroyThreadPrivate
   to free the thread-private data because that function takes care
   of all the necessary iterations (4) of invoking the destructors.
4. prtpd.c: Removed unused macro _PR_TPD_MODULO and made _pr_tpd_destructors
   static.  PR_NewThreadPrivateIndex should return index starting from
   0 as opposed to 1.  Fixed several problems in PR_SetThreadPrivate and
   _PR_DestroyThreadPrivate.  Also, _PR_DestroyThreadPrivate now doesn't
   free self->privateData, so that it can be called by _PR_CleanupThread
   to prepare a dead thread for recycling.
5. pruthr.c: _PR_NativeDestroyThread should free thread->privateData
   to plug the memory leak that chouck reported.

Comment 5

[Wan-Teh Chang]

I checked in the fix.
/cvsroot/mozilla/nsprpub/pr/include/private/primpl.h, revision 3.27
/cvsroot/mozilla/nsprpub/pr/src/pthreads/ptthread.c, revision 3.28
/cvsroot/mozilla/nsprpub/pr/src/threads/prcthr.c, revision 3.10
/cvsroot/mozilla/nsprpub/pr/src/threads/prtpd.c, revision 3.7
/cvsroot/mozilla/nsprpub/pr/src/threads/combined/pruthr.c, revision 3.16

The fix is also checked into the internal repository.
/m/src/ns/nspr20/pr/include/private/primpl.h, revision 2.68
/m/src/ns/nspr20/pr/src/pthreads/ptthread.c, revision 2.56
/m/src/ns/nspr20/pr/src/threads/prcthr.c, revision 2.15
/m/src/ns/nspr20/pr/src/threads/prtpd.c, revision 2.10
/m/src/ns/nspr20/pr/src/threads/combined/pruthr.c, revision 2.33

Chouck, can you verify that the memory leak is gone?  Thanks.

Comment 6

[chouck]

After applying your patch Purify is no longer reporting leaked thread
private data.

Thanks,
-Chris

Comment 7

[Wan-Teh Chang]

Thanks, Chris.  Marked the bug verified.

Comment 8

[srinivas]

The patch looks good.

In _pt_thread_death, shouldn't thred->errorString, thred->io_cv and
thred->privateData be set to NULL?

Comment 9

[Wan-Teh Chang]

Setting those fields of 'thred' to NULL is not
necessary because 'thred' will be freed.

Comment 10

[srinivas]

This isn't a bug, I was suggesting it more as a safety feature, if the threads
are recycled in the feature. When resources are freed conditionally, it is
better to reset the condition after freeing the resources.