Closed Bug 372876 Opened 14 years ago Closed 13 years ago

Read from SSL socket leaks memory

Categories

(Core :: Security: PSM, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED

People

(Reporter: swsnyder, Assigned: KaiE)

References

()

Details

(Keywords: memory-leak, Whiteboard: [kerh-coa])

Attachments

(2 files)

Rational's Purify consistently reports memory leaks when reading HTTPS web pages.  

The trace is below.  I've exploded the first few routines in the call stack to illustrate the lost (re)allocations.

How to reproduce:

1. Open SeaMonkey v1.1.1 to a blank page.
2. Go to https://bugzilla.mozilla.org/.  Wait for page load to complete.
3. Exit SeaMonkey.


[W] MLK: Memory leak of 20480 bytes from 5 blocks allocated in NS_Realloc_P [xpcom_core.dll]
    Distribution of leaked blocks
    Allocation location
    realloc        [f:\vs70builds\6030\vc\crtbld\crt\src\stdenvp.c:64]
    NS_Realloc_P   [c:\mozbuild\mozilla\xpcom\base\nsmemoryimpl.cpp:349]
    NS_Realloc(void* ptr, PRSize size)
    {
        NS_ASSERTION(size, "NS_Realloc of size 0");
 =>     void* result = REALLOC1(ptr, size);
        if (! result) {
            // Request an asynchronous flush
            sGlobalMemory.FlushMemory(NS_LITERAL_STRING("alloc-failure").get(), PR_FALSE);
    nsSSLSocketThreadData::ensure_buffer_size(int) [c:\mozbuild\mozilla\security\manager\ssl\src\nsnssiolayer.cpp:164]
    {
      if (amount > mSSLDataBufferAllocatedSize) {
        if (mSSLDataBuffer) {
 =>       mSSLDataBuffer = (char*)nsMemory::Realloc(mSSLDataBuffer, amount);
        }
        else {
          mSSLDataBuffer = (char*)nsMemory::Alloc(amount);
    nsSSLThread::requestRead(nsNSSSocketInfo *,void *,int) [c:\mozbuild\mozilla\security\manager\ssl\src\nssslthread.cpp:580]
      // si is idle and good, and no other socket is currently busy,
      // so it's fine to continue with the request.
    
 =>   if (!si->mThreadData->ensure_buffer_size(amount))
      {
        PR_SetError(PR_OUT_OF_MEMORY_ERROR, 0);
        return -1;
    nsSSLIOLayerRead [c:\mozbuild\mozilla\security\manager\ssl\src\nsnssiolayer.cpp:1209]
          nsNSSSocketInfo *socketInfo = (nsNSSSocketInfo*)fd->secret;
          NS_ASSERTION(socketInfo,"nsNSSSocketInfo was null for an fd");
        
     =>   return nsSSLThread::requestRead(socketInfo, buf, amount);
        }
        
        static PRInt32 PR_CALLBACK
    PR_Read        [c:\mozbuild\mozilla\nsprpub\pr\src\io\priometh.c:141]
    nsSocketInputStream::Read(char *,UINT,UINT *) [c:\mozbuild\mozilla\netwerk\base\src\nssockettransport2.cpp:338]
    nsHttpConnection::OnWriteSegment(char *,UINT,UINT *) [c:\mozbuild\mozilla\netwerk\protocol\http\src\nshttpconnection.cpp:617]
    nsHttpTransaction::WritePipeSegment(nsIOutputStream *,void *,char *,UINT,UINT,UINT *) [c:\mozbuild\mozilla\netwerk\protocol\http\src\nshttptransaction.cpp:471]
    nsPipeOutputStream::WriteSegments((*)(nsIOutputStream *,void *,char *,UINT,UINT,UINT *),void *,UINT,UINT *) [c:\mozbuild\mozilla\xpcom\io\nspipe3.cpp:1100]
Hmmm...  It may have been a mistake to explode the call stack.  It's probably heard to see the sequence from the above output.  Here's a more concise view of the same events:

[W] MLK: Memory leak of 20480 bytes from 5 blocks allocated in NS_Realloc_P [xpcom_core.dll]
        Distribution of leaked blocks
        Allocation location
            realloc        [f:\vs70builds\6030\vc\crtbld\crt\src\stdenvp.c:64]
            NS_Realloc_P   [c:\mozbuild\mozilla\xpcom\base\nsmemoryimpl.cpp:349]
            nsSSLSocketThreadData::ensure_buffer_size(int) [c:\mozbuild\mozilla\security\manager\ssl\src\nsnssiolayer.cpp:164]
            nsSSLThread::requestRead(nsNSSSocketInfo *,void *,int) [c:\mozbuild\mozilla\security\manager\ssl\src\nssslthread.cpp:580]
            nsSSLIOLayerRead [c:\mozbuild\mozilla\security\manager\ssl\src\nsnssiolayer.cpp:1209]
            PR_Read        [c:\mozbuild\mozilla\nsprpub\pr\src\io\priometh.c:141]
            nsSocketInputStream::Read(char *,UINT,UINT *) [c:\mozbuild\mozilla\netwerk\base\src\nssockettransport2.cpp:338]
            nsHttpConnection::OnWriteSegment(char *,UINT,UINT *) [c:\mozbuild\mozilla\netwerk\protocol\http\src\nshttpconnection.cpp:617]
            nsHttpTransaction::WritePipeSegment(nsIOutputStream *,void *,char *,UINT,UINT,UINT *) [c:\mozbuild\mozilla\netwerk\protocol\http\src\nshttptransaction.cpp:471]
            nsPipeOutputStream::WriteSegments((*)(nsIOutputStream *,void *,char *,UINT,UINT,UINT *),void *,UINT,UINT *) [c:\mozbuild\mozilla\xpcom\io\nspipe3.cpp:1100]


The leak is very consistent.  The more HTTPS pages viewed, the more leaks are reported at application exit.
Reclassifying this bug.
The code cited in this bug is PSM code, not NSS code. 
Assignee: nobody → kengert
Component: Libraries → Security: PSM
Product: NSS → Core
QA Contact: libraries
Version: unspecified → 1.8 Branch
Whiteboard: [kerh-coa]
QA Contact: psm
This happens on trunk as well.

I did a trace-malloc diffbloatdump analysis on sunbird (trunk, Linux) to figure out why it was leaking so much -- and one of the things that showed up was allocations with the above stack -- reloading my calendars every 2 minutes, it added up to hundreds of kilobytes after less than an hour.

As far as I can tell, nobody ever frees mSSLDataBuffer.
Flags: blocking1.9?
Keywords: mlk
OS: Windows XP → All
Hardware: PC → All
Version: 1.8 Branch → Trunk
David, thanks so much for identifying the cause.
You're absolutely right. I can't believe I missed that. Sigh!
Attached patch Patch v1Splinter Review
Attachment #304071 - Flags: review?(rrelyea)
Attachment #304071 - Flags: approval1.8.1.13?
Flags: blocking1.9? → blocking1.9+
Duplicate of this bug: 391892
Why not just use nsTArray<char> for this buffer? Would save lots of code and prevent leaks.
Comment on attachment 304071 [details] [diff] [review]
Patch v1

r+ rrelyea
Attachment #304071 - Flags: review?(rrelyea) → review+
Attachment #304071 - Flags: approval1.8.1.13?
Checked in to get this fixed asap.

I should leave this bug open and look into Robert's proposal in a follow up patch.
Might want to just have a followup mini-project to use nsTArray everywhere in PSM it makes sense.
(In reply to comment #11)
> Might want to just have a followup mini-project to use nsTArray everywhere in
> PSM it makes sense.

Filed bug 419887.
Marking this one fixed.

Status: NEW → RESOLVED
Closed: 13 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.