If you think a bug might affect users in the 57 release, please set the correct tracking and status flags for Release Management.

Read from SSL socket leaks memory

RESOLVED FIXED

Status

()

Core
Security: PSM
RESOLVED FIXED
11 years ago
10 years ago

People

(Reporter: Steve Snyder, Assigned: kaie)

Tracking

({mlk})

Trunk
Points:
---
Bug Flags:
blocking1.9 +

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [kerh-coa], URL)

Attachments

(2 attachments)

(Reporter)

Description

11 years ago
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]
(Reporter)

Comment 1

11 years ago
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.
(Reporter)

Comment 2

11 years ago
Created attachment 257550 [details]
Full Purify report of memory leak.
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
(Assignee)

Updated

11 years ago
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
(Assignee)

Comment 5

10 years ago
David, thanks so much for identifying the cause.
You're absolutely right. I can't believe I missed that. Sigh!
(Assignee)

Comment 6

10 years ago
Created attachment 304071 [details] [diff] [review]
Patch v1
Attachment #304071 - Flags: review?(rrelyea)
(Assignee)

Updated

10 years ago
Attachment #304071 - Flags: approval1.8.1.13?

Updated

10 years ago
Flags: blocking1.9? → blocking1.9+
(Assignee)

Updated

10 years ago
Duplicate of this bug: 391892
Why not just use nsTArray<char> for this buffer? Would save lots of code and prevent leaks.

Comment 9

10 years ago
Comment on attachment 304071 [details] [diff] [review]
Patch v1

r+ rrelyea
Attachment #304071 - Flags: review?(rrelyea) → review+
(Assignee)

Updated

10 years ago
Attachment #304071 - Flags: approval1.8.1.13?
(Assignee)

Comment 10

10 years ago
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.
(Assignee)

Comment 12

10 years ago
(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
Last Resolved: 10 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.