Closed Bug 400957 Opened 12 years ago Closed 12 years ago

Memory allocated for logBuf in PR_SetLogBuffering is never freed.

Categories

(NSPR :: NSPR, defect)

defect
Not set

Tracking

(Not tracked)

RESOLVED FIXED

People

(Reporter: slavomir.katuscak+mozilla, Assigned: wtc)

Details

(Keywords: memory-leak)

Attachments

(1 file)

Block in use (biu):
Found block of size 16384 bytes at address 0x85168e0 (81.37% of total)
At time of allocation, the call stack was:
    [1] PR_Malloc() at line 467 in "prmem.c"
    [2] PR_SetLogBuffering() at line 436 in "prlog.c"
    [3] _PR_InitLog() at line 256 in "prlog.c"
    [4] _PR_InitStuff() at line 241 in "prinit.c"
    [5] _PR_ImplicitInitialization() at line 259 in "prinit.c"
    [6] PR_GetSpecialFD() at line 1215 in "ptio.c"
    [7] main() at line 1006 in "ocspclnt.c"
Stacks in OPT version are a bit shorter, but still the same problem:
selfserv/main/PR_Init/_PR_InitLog/PR_Malloc
strsclnt/main/PR_Init/_PR_InitLog/PR_Malloc
ocspclnt/main/PR_GetSpecialFD/_PR_ImplicitInitialization/_PR_InitLog/PR_Malloc

Thanks for the bug report, Slavo.

I took the opportunity to verify that _PR_LogCleanup frees all the static
variables in this file (_pr_logLock, logModules, logBuf, and logFile).
Attachment #286023 - Flags: review?(kengert)
Looking at the uses of logBuf, I see a potential crash.
PR_SetLogBuffering does
    if (logBuf)
        PR_DELETE(logBuf);

But it might not set a new buffer.

I propose, whenever deleting logBuf, you should do
  logBuf = NULL;


Should we the same = NULL in your patch, too?


Should PR_SetLogBuffering use _PR_LOCK_LOG() / _PR_UNLOCK_LOG() ?
Good question, Kai.  PR_DELETE is a macro -- it calls PR_Free and
then sets the pointer to NULL.  So it does what you suggested.
Comment on attachment 286023 [details] [diff] [review]
Proposed patch: free logBuf in _PR_LogCleanup

Thanks Wan-Teh.

r=kengert
Attachment #286023 - Flags: review?(kengert) → review+
PR_SetLogBuffering and PR_SetLogFile should use
_PR_LOCK_LOG() / _PR_UNLOCK_LOG().  However, we can't just
enclose the functions' bodies with _PR_LOCK_LOG() / _PR_UNLOCK_LOG().
Specifically, we can't lock around calls to other NSPR functions
(e.g., PR_Open) because they may perform logging, which would result
in a deadlock.  So the patch to fix this could be a little complicated.
I suggest that we address the locking issue in a separate bug.

I checked in my patch on the NSPR trunk for NSPR 4.7.

Checking in prlog.c;
/cvsroot/mozilla/nsprpub/pr/src/io/prlog.c,v  <--  prlog.c
new revision: 3.40; previous revision: 3.39
done
Status: NEW → RESOLVED
Closed: 12 years ago
Resolution: --- → FIXED
Target Milestone: --- → 4.7
You need to log in before you can comment on or make changes to this bug.