372876 - Read from SSL socket leaks memory

Status: RESOLVED FIXED

(Core :: Security: PSM, defect)

Description

[Steve Snyder]

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]

Comment 1

[Steve Snyder]

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.

Comment 2

[Steve Snyder]

Attachment: Full Purify report of memory leak.

Comment 3

[Nelson Bolyard (seldom reads bugmail)]

Reclassifying this bug.
The code cited in this bug is PSM code, not NSS code. 

Comment 4

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

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.

Comment 5

[Kai Engert (:KaiE:)]

David, thanks so much for identifying the cause.
You're absolutely right. I can't believe I missed that. Sigh!

Comment 6

[Kai Engert (:KaiE:)]

Attachment: Patch v1

Comment 8

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Why not just use nsTArray<char> for this buffer? Would save lots of code and prevent leaks.

Comment 9

[Robert Relyea]

Comment on attachment 304071 [details] [diff] [review]
Patch v1

r+ rrelyea

Comment 10

[Kai Engert (:KaiE:)]

Checked in to get this fixed asap.

I should leave this bug open and look into Robert's proposal in a follow up patch.

Comment 11

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Might want to just have a followup mini-project to use nsTArray everywhere in PSM it makes sense.

Comment 12

[Kai Engert (:KaiE:)]

(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.