Closed Bug 103870 Opened 18 years ago Closed 6 years ago

unsafe realloc usage in nsMsgSendLater allows memory leak under low memory conditions

Categories

(MailNews Core :: Composition, defect, minor)

defect
Not set
minor

Tracking

(Not tracked)

RESOLVED FIXED
Thunderbird 25.0

People

(Reporter: Geoffrey.R.Gustafson, Assigned: aceman)

References

(Blocks 2 open bugs)

Details

(Keywords: memory-leak)

Attachments

(3 files, 5 obsolete files)

In compose/src/nsMsgSendLater.cpp, the BuildNewBuffer() function:

249   PRInt32 leftoverSize = PL_strlen(mLeftoverBuffer);
250   mLeftoverBuffer = (char *)PR_Realloc(mLeftoverBuffer, aCount + 
leftoverSize);
251   if (!mLeftoverBuffer)
252     return NS_ERROR_FAILURE;

This usage of realloc: a = realloc(a, size) is a common bug (described in the 
book Writing Solid Code).

If realloc returns null, the original pointer is overwritten and lost, hence a 
memory leak. Instead, it should be something like:

char *result = (char *)PR_Realloc(mLeftoverBuffer, aCount + leftoverSize);
if (result)
  mLeftoverBuffer = result;
else return NS_ERROR_FAILURE;

Or, as I believe Steve Maguire suggests in the book, you could wrap realloc 
with a function that separates the error condition from the result, e.g.

PRBool PR_Realloc(char **aBuffer, PRUint32 aCount);

It would be great to do that in NSPR itself as shown, to prevent the bug 
throughout all of Mozilla.
putting in the right component.
Assignee: mscott → ducarroz
Component: Mail Back End → Composition
QA Contact: esther → sheelar
Target Milestone: --- → mozilla0.9.6
Admittedly, this bug would only occur under low memory conditions. If you 
search for PR_Realloc in LXR/text search the bug appears to be present in 
several dozen places (some of them might have made a backup copy of the 
variable). So, someone could go on a warpath.
QA Contact: sheelar → stephend
moving to 1.0
Target Milestone: mozilla0.9.6 → mozilla1.0
*** Bug 103947 has been marked as a duplicate of this bug. ***
Bugs targeted at mozilla1.0 without the mozilla1.0 keyword moved to mozilla1.0.1 
(you can query for this string to delete spam or retrieve the list of bugs I've 
moved)
Target Milestone: mozilla1.0 → mozilla1.0.1
Status: NEW → ASSIGNED
Product: MailNews → Core
Assignee: ducarroz → nobody
Status: ASSIGNED → NEW
QA Contact: stephend → composition
Product: Core → MailNews Core
Keywords: mlk
Severity: normal → minor
Target Milestone: mozilla1.0.1 → ---
Looks like this is still valid:
mxr.mozilla.org/comm-central/search?string=PR_realloc&tree=comm-central

The spots where VAR=PR_realloc(VAR, size) are clearly seen.
It looks like they have this mostly covered in /mozilla/* . But there are meny spots in /mailnews/*.

I think I could convert all the spots, I just need the decision if I should make that wrapper function (if possible due to pointer types) or just open-code all the checks at all places.
Assignee: nobody → acelists
OS: Linux → All
Hardware: x86 → All
So aiui this will really only affect cases where we're hitting out of memory, and if we do, then we'll just make the situation slightly worse.

(In reply to :aceman from comment #6)
> I think I could convert all the spots, I just need the decision if I should
> make that wrapper function (if possible due to pointer types) or just
> open-code all the checks at all places.

I think it needs to be on a case by-case basis. For example, looking in nsPop3Sink at the m_outputBuffer usage, makes we think we should just convert m_outputBuffer to an nsCString and have the string class do all the work for us.
OK. I can't yet play these ns*String games.
Assignee: acelists → nobody
how much do we leak?  A few bytes?
Summary: unsafe realloc usage allows memory leak → unsafe realloc usage in nsMsgSendLater allows memory leak
Wayne, see comment 2. We do not leak anything. Only if there is not enough memory in the system (including swap). And then we may leak some bytes, but we'll probably crash/exit soon after that as there is not enough memory for more critical operations.

Standard8, aren't the nsCString operations much slower than direct alloc/memcpy?
Assignee: nobody → acelists
Flags: needinfo?(mbanner)
I can't comment on the comparative speed of the operations. I do know that using nsCString & co is a much better way of ensuring you don't leak memory (and they are used probably more widely than manual string management functions these days).
Flags: needinfo?(mbanner)
Summary: unsafe realloc usage in nsMsgSendLater allows memory leak → unsafe realloc usage in nsMsgSendLater allows memory leak under low memory conditions
Attached patch patch for mime (obsolete) — Splinter Review
Attachment #714680 - Flags: review?(Pidgeot18)
Attached patch patch for mailnews (obsolete) — Splinter Review
Attachment #714682 - Flags: review?(mbanner)
I will note offhand that we are now in a world where malloc-failure causes the program to hard abort, so the realloc failure won't cause any leaks.
That is not written in the comments around PR_MALLOC/REALLOC. It says they just forward to the standard libc functions (malloc/realloc). Those do not abort. 
What you say may be true about allocations where there is m-c code around them, e.g. lengthening ns*String. Are you sure about this?
Comment on attachment 714682 [details] [diff] [review]
patch for mailnews

Review of attachment 714682 [details] [diff] [review]:
-----------------------------------------------------------------

I must admit I'm not entirely sure we should be using the PR_ form in all these cases, so I'm going to pass this over to Neil.
Attachment #714682 - Flags: review?(mbanner) → review?(neil)
(In reply to :aceman from comment #15)
> That is not written in the comments around PR_MALLOC/REALLOC. It says they
> just forward to the standard libc functions (malloc/realloc). Those do not
> abort. 
> What you say may be true about allocations where there is m-c code around
> them, e.g. lengthening ns*String. Are you sure about this?

After paying careful attention, it's ::operator new that is always infallible; there are intents to make malloc and friends infallible too, but that first needs a rewriting effort to make them use the infallible variants.

(In reply to Mark Banner (:standard8) from comment #16)
> I must admit I'm not entirely sure we should be using the PR_ form in all
> these cases, so I'm going to pass this over to Neil.

My understanding is that using the NSPR allocation functions is necessary, since the plain libc functions end up using the wrong heap on Windows.
Comment on attachment 714680 [details] [diff] [review]
patch for mime

Review of attachment 714680 [details] [diff] [review]:
-----------------------------------------------------------------

I'd like to see nsMimeRebuffer using an nsCString instead, and mimemalt using nsTArrays (actually, nsAutoTArray<..., 2> would probably be better) instead. The mimemrel and nsMsgHeaderParser are too far done the rathole of char*-manipulation to be worth changing to better C++ constructs.
Attachment #714680 - Flags: review?(Pidgeot18) → review-
Comment on attachment 714682 [details] [diff] [review]
patch for mailnews

> static char * lexGetDataFromBase64()
We really should replace this with one of our other base64 decoders.

>+  m_dataBuffer = newBuffer;
>   m_dataBufferSize = desiredSize;
m_dataBuffer/m_dataBufferSize should become an nsTArray.
nsByteArray, if we still use it, should too.

>+    char* newDestination = (char*) PR_Realloc(*destination, destLength + PL_strlen(source) + 1);
Is PR_Realloc null-safe?

>-  /* realloc it smaller... */
>-  buffer = (char*)PR_REALLOC (buffer, buffer_tail - buffer + 1);
If the comment is true, this is infallible already.

>+  char* newBuffer = (char *) PR_Realloc(mLeftoverBuffer, aCount + leftoverSize);
>+  NS_ENSURE_TRUE(newBuffer, NS_ERROR_OUT_OF_MEMORY);
>+  mLeftoverBuffer = newBuffer;
> 
>   memcpy(mLeftoverBuffer + leftoverSize, aBuf, aCount);
This should probably become an nsTArray too.

>+    m_copyState->m_dataBuffer = newBuffer;
>     m_copyState->m_dataBufferSize = aLength + m_copyState->m_leftOver;
As should these.

>             if (m_nsIPop3Sink)
>-                m_nsIPop3Sink->SetMailAccountURL(NULL);
...
> nsresult
>-nsPop3Sink::SetMailAccountURL(const char* urlString)
>+nsPop3Sink::SetMailAccountURL(const nsACString &urlString)
> {
>-  if (urlString)
Interesting... not sure what to advise here.

>-    PR_smprintf_free(statusLine);
Leak.

>-nsresult nsPop3Sink::WriteLineToMailbox(const char *buffer)
>+nsresult nsPop3Sink::WriteLineToMailbox(nsACString& buffer)
Not quite sure why you changed this, since some of the calls looked awkward, and then I forgot to go back and double-check.

>+      m_newMailParser->HandleLine((char*)PromiseFlatCString(buffer).get(), bufferLen);
buffer.BeginWriting()

>+    m_outFileStream->Write(PromiseFlatCString(buffer).get(), bufferLen, &bytesWritten);
buffer.BeginReading()

>-  nsresult rv = WriteLineToMailbox(MSG_LINEBREAK);
>+  nsCString lineBreak(MSG_LINEBREAK);
>+  nsresult rv = WriteLineToMailbox(lineBreak);
NS_LITERAL_CSTRING(MSG_LINEBREAK)
Attachment #714682 - Flags: review?(neil) → review-
I'm not sure why this grew into a reworking of the variables. This may grow the patches 10x.

(In reply to neil@parkwaycc.co.uk from comment #19)
> > static char * lexGetDataFromBase64()
> We really should replace this with one of our other base64 decoders.
> 
> >+  m_dataBuffer = newBuffer;
> >   m_dataBufferSize = desiredSize;
> m_dataBuffer/m_dataBufferSize should become an nsTArray.
> nsByteArray, if we still use it, should too.
nsTArray of what type?

> 
> >+    char* newDestination = (char*) PR_Realloc(*destination, destLength + PL_strlen(source) + 1);
> Is PR_Realloc null-safe?
???

> >-  /* realloc it smaller... */
> >-  buffer = (char*)PR_REALLOC (buffer, buffer_tail - buffer + 1);
> If the comment is true, this is infallible already.
How do you know? Is it mandated that all implementations must just shrink the allocated memory in place and never copy it to other place even if it is smaller?
Attached patch patch for mime v2 (obsolete) — Splinter Review
Attachment #714680 - Attachment is obsolete: true
Attachment #738721 - Flags: review?(Pidgeot18)
Comment on attachment 738721 [details] [diff] [review]
patch for mime v2

Review of attachment 738721 [details] [diff] [review]:
-----------------------------------------------------------------

Several nits:

::: mailnews/mime/emitters/src/nsMimeBaseEmitter.h
@@ +89,5 @@
>    nsresult            DumpToCC();
>    nsresult            DumpRestOfHeaders();
>    nsresult            OutputGenericHeader(const char *aHeaderVal);
>  
> +  nsresult            WriteHelper(const nsACString &buf, uint32_t count, uint32_t *countWritten);

Lose the count parameter; it's implied by buf.Length().

::: mailnews/mime/emitters/src/nsMimeRebuffer.h
@@ +22,2 @@
>      uint32_t      ReduceBuffer(uint32_t numBytes);
> +    nsCString &   GetBuffer();

const nsACString &

::: mailnews/mime/src/mimemalt.cpp
@@ +279,5 @@
>    int32_t i = malt->pending_parts++;
>    if (malt->pending_parts > malt->max_parts) {
>      malt->max_parts = malt->pending_parts;
> +    char* newBuf = (char*) PR_REALLOC(malt->buffered_hdrs, malt->max_parts *
> +                                      sizeof(*malt->buffered_hdrs));

Don't cast to char * here, use MimeHeaders** instead.

@@ +284,5 @@
> +    NS_ENSURE_TRUE(newBuf, MIME_OUT_OF_MEMORY);
> +    malt->buffered_hdrs = (MimeHeaders **) newBuf;
> +
> +    newBuf = (char*) PR_REALLOC(malt->part_buffers, malt->max_parts *
> +                                sizeof(*malt->part_buffers));

And MimePartBufferData** here.

::: mailnews/mime/src/nsMsgHeaderParser.cpp
@@ +1564,5 @@
> +  if (!newBuf) {
> +    PR_FREEIF(buf);
> +    newBuf = 0;
> +  }
> +  return newBuf;

This is a shrinking allocation, which means both that it is very unlikely to fail (peeking at the source code for jemalloc, I think it is actually impossible for it to fail!) and that your fallback code here is wrong.

However, since I already have patches awaiting review that would remove this file altogether, just lose this change altogether.
Attachment #738721 - Flags: review?(Pidgeot18) → review+
Thanks, done.
Attachment #738721 - Attachment is obsolete: true
Attachment #745638 - Flags: review+
Status: NEW → ASSIGNED
Keywords: checkin-needed
Whiteboard: [check in only the marked patch, leave open after checkin]
https://hg.mozilla.org/comm-central/rev/14e6ef96bf28
Keywords: checkin-needed
Whiteboard: [check in only the marked patch, leave open after checkin] → [leave open]
Attachment #745638 - Attachment description: patch for mime v3 [ready for check-in] → patch for mime v3 [checkin: comment 24]
Truncate(0) -> Truncate()
Attachment #748458 - Flags: review?(neil)
Comment on attachment 748458 [details] [diff] [review]
[checked in] external api fix

Sorry for the delay, bug 866365 somehow busted my external build.
Attachment #748458 - Flags: review?(neil) → review+
Comment on attachment 748458 [details] [diff] [review]
[checked in] external api fix

No problem - pushed as https://hg.mozilla.org/comm-central/rev/43c50335022a
Attachment #748458 - Attachment description: external api fix → [checked in] external api fix
Neil, I'd need reply to comment 20 to continue here.
Flags: needinfo?(neil)
(In reply to aceman from comment #20)
> (In reply to comment #19)
> > >+  m_dataBuffer = newBuffer;
> > >   m_dataBufferSize = desiredSize;
> > m_dataBuffer/m_dataBufferSize should become an nsTArray.
> > nsByteArray, if we still use it, should too.
> nsTArray of what type?
nsTArray<char> I assume.

> > >+    char* newDestination = (char*) PR_Realloc(*destination, destLength + PL_strlen(source) + 1);
> > Is PR_Realloc null-safe?
> ???
Probably. Apparently it was specified as safe in ISO C.

> > >-  /* realloc it smaller... */
> > >-  buffer = (char*)PR_REALLOC (buffer, buffer_tail - buffer + 1);
> > If the comment is true, this is infallible already.
> How do you know? Is it mandated that all implementations must just shrink
> the allocated memory in place and never copy it to other place even if it is
> smaller?
You're right, we don't guarantee to be able to shrink an allocation... seems like an oversight in jemalloc though; the gnu libc realloc is only documented to fail to increase the allocation.
Flags: needinfo?(neil)
(In reply to neil@parkwaycc.co.uk from comment #19)
> >+  m_dataBuffer = newBuffer;
> >   m_dataBufferSize = desiredSize;
> m_dataBuffer/m_dataBufferSize should become an nsTArray.
> nsByteArray, if we still use it, should too.
Could this be done in a new bug?

> >-  nsresult rv = WriteLineToMailbox(MSG_LINEBREAK);
> >+  nsCString lineBreak(MSG_LINEBREAK);
> >+  nsresult rv = WriteLineToMailbox(lineBreak);
> NS_LITERAL_CSTRING(MSG_LINEBREAK)
This does not work.
Flags: needinfo?(neil)
Attached patch patch for mailnews v2 (obsolete) — Splinter Review
Attachment #714682 - Attachment is obsolete: true
Attachment #769493 - Flags: review?(neil)
(In reply to aceman from comment #30)
> (In reply to comment #19)
> > >+  m_dataBuffer = newBuffer;
> > >   m_dataBufferSize = desiredSize;
> > m_dataBuffer/m_dataBufferSize should become an nsTArray.
> > nsByteArray, if we still use it, should too.
> Could this be done in a new bug?
I guess so.

> > >-  nsresult rv = WriteLineToMailbox(MSG_LINEBREAK);
> > >+  nsCString lineBreak(MSG_LINEBREAK);
> > >+  nsresult rv = WriteLineToMailbox(lineBreak);
> > NS_LITERAL_CSTRING(MSG_LINEBREAK)
> This does not work.
What error do you get?
Flags: needinfo?(neil)
Blocks: 888754
Blocks: 888755
(In reply to neil@parkwaycc.co.uk from comment #32)
> > > >-  nsresult rv = WriteLineToMailbox(MSG_LINEBREAK);
> > > >+  nsCString lineBreak(MSG_LINEBREAK);
> > > >+  nsresult rv = WriteLineToMailbox(lineBreak);
> > > NS_LITERAL_CSTRING(MSG_LINEBREAK)
> > This does not work.
> What error do you get?

mailnews/local/src/nsPop3Sink.cpp:759:150: error: no matching function for call to 'nsPop3Sink::WriteLineToMailbox(const nsDependentCString&)'
   nsresult rv = WriteLineToMailbox(NS_LITERAL_CSTRING(MSG_LINEBREAK));
mailnews/local/src/nsPop3Sink.cpp:759:150: note: candidate is:
mailnews/local/src/nsPop3Sink.cpp:678:10: note: nsresult nsPop3Sink::WriteLineToMailbox(nsACString_internal&)
 nsresult nsPop3Sink::WriteLineToMailbox(nsACString& buffer)
mailnews/local/src/nsPop3Sink.cpp:678:10: note:   no known conversion for argument 1 from 'const nsDependentCString' to 'nsACString_internal&'
Flags: needinfo?(neil)
Comment on attachment 769493 [details] [diff] [review]
patch for mailnews v2

> 			oldBytes = bytes;
>-			bytes = (unsigned char*)PR_Realloc(bytes,bytesMax);
>-			}
>-			if (bytes == 0) {
>-			  mime_error("out of memory while processing BASE64 data\n");
>-                          break;
>-			}
> 		    }
>+		    bytes = (unsigned char*)PR_Realloc(oldBytes, bytesMax);
So, what's the point of this change? The old code was already saving the old pointer, and the code is there to free it up if necessary.

>+  /* realloc it smaller... */
>+  char *newBuffer = (char*) PR_REALLOC(buffer, buffer_tail - buffer + 1);
>+  if (!newBuffer) {
>+    PR_FREEIF(buffer);
>+    *status = NS_ERROR_OUT_OF_MEMORY;
>+    return nullptr;
In theory we could just return the old buffer if the realloc fails.

>-                m_nsIPop3Sink->SetMailAccountURL(NULL);
>+                m_nsIPop3Sink->SetMailAccountURL(NS_LITERAL_CSTRING(""));
I have no idea what this does or whether anyone users it. I don't mind if you just remove this call to make the back end easier.

>+  if (buffer.Length())
Nit: Always use IsEmpty() for boolean tests.

>     if (m_newMailParser) // HandleLine should really take a const char *...
It doesn't actually write to its parameter, so it would be trivial to redeclare the function...

>+      m_newMailParser->HandleLine(buffer.BeginWriting(), bufferLen);
In which case this would become BeginReading of course.
(In reply to aceman from comment #33)
> (In reply to comment #32)
> > > > >-  nsresult rv = WriteLineToMailbox(MSG_LINEBREAK);
> > > > >+  nsCString lineBreak(MSG_LINEBREAK);
> > > > >+  nsresult rv = WriteLineToMailbox(lineBreak);
> > > > NS_LITERAL_CSTRING(MSG_LINEBREAK)
> > > This does not work.
> > What error do you get?
> 
> mailnews/local/src/nsPop3Sink.cpp:759:150: error: no matching function for
> call to 'nsPop3Sink::WriteLineToMailbox(const nsDependentCString&)'
>    nsresult rv = WriteLineToMailbox(NS_LITERAL_CSTRING(MSG_LINEBREAK));
> mailnews/local/src/nsPop3Sink.cpp:759:150: note: candidate is:
> mailnews/local/src/nsPop3Sink.cpp:678:10: note: nsresult
> nsPop3Sink::WriteLineToMailbox(nsACString_internal&)
>  nsresult nsPop3Sink::WriteLineToMailbox(nsACString& buffer)
> mailnews/local/src/nsPop3Sink.cpp:678:10: note:   no known conversion for
> argument 1 from 'const nsDependentCString' to 'nsACString_internal&'

Ah of course, this is all HandleLine's fault - if you make its parameter const then you can make WriteLineToMailbox take a const parameter too.
Flags: needinfo?(neil)
(In reply to neil@parkwaycc.co.uk from comment #34)
> Comment on attachment 769493 [details] [diff] [review]
> patch for mailnews v2
> 
> > 			oldBytes = bytes;
> >-			bytes = (unsigned char*)PR_Realloc(bytes,bytesMax);
> >-			}
> >-			if (bytes == 0) {
> >-			  mime_error("out of memory while processing BASE64 data\n");
> >-                          break;
> >-			}
> > 		    }
> >+		    bytes = (unsigned char*)PR_Realloc(oldBytes, bytesMax);
> So, what's the point of this change? The old code was already saving the old
> pointer, and the code is there to free it up if necessary.
It just merges 2 different allocation function calls (PR_CALLOC and PR_Realloc) into one. Similar to the change in NS_MsgSACat.
Attached patch patch for mailnews v3 (obsolete) — Splinter Review
Attachment #769493 - Attachment is obsolete: true
Attachment #769493 - Flags: review?(neil)
Attachment #769867 - Flags: review?(neil)
Comment on attachment 769867 [details] [diff] [review]
patch for mailnews v3

>+		    bytes = (unsigned char*)PR_Realloc(oldBytes, bytesMax);
Nit: everywhere else you put in a space after the cast.

>-    nsCOMPtr <nsINntpService> nntpService = do_GetService("@mozilla.org/messenger/nntpservice;1");
>+    nsCOMPtr<nsINntpService> nntpService = do_GetService("@mozilla.org/messenger/nntpservice;1", &rv);
>     if (NS_FAILED(rv) || !nntpService) {
Wow!

>+    outputString.AssignLiteral("X-Mozilla-Status2: 00000000" MSG_LINEBREAK);
>+    rv = WriteLineToMailbox(outputString);
...
>+    outputString.AssignLiteral(X_MOZILLA_KEYWORDS);
>+    rv = WriteLineToMailbox(outputString);
[I wonder whether these can be done with an NS_LITERAL_CSTRING]

>+    m_outputBuffer.AssignLiteral(">");
Nit: Could Append('>') instead.
Attachment #769867 - Flags: review?(neil) → review+
(In reply to neil@parkwaycc.co.uk from comment #38)
> >-    nsCOMPtr <nsINntpService> nntpService = do_GetService("@mozilla.org/messenger/nntpservice;1");
> >+    nsCOMPtr<nsINntpService> nntpService = do_GetService("@mozilla.org/messenger/nntpservice;1", &rv);
> >     if (NS_FAILED(rv) || !nntpService) {
> Wow!
Is this a positive wow? :)


> >+    outputString.AssignLiteral("X-Mozilla-Status2: 00000000" MSG_LINEBREAK);
> >+    rv = WriteLineToMailbox(outputString);
> ...
> >+    outputString.AssignLiteral(X_MOZILLA_KEYWORDS);
> >+    rv = WriteLineToMailbox(outputString);
> [I wonder whether these can be done with an NS_LITERAL_CSTRING]

Yes, maybe these will work now after the 'const' conversions.
(In reply to neil@parkwaycc.co.uk from comment #38)
> >+    m_outputBuffer.AssignLiteral(">");
> Nit: Could Append('>') instead.
Why would this be better?
Flags: needinfo?(neil)
(In reply to aceman from comment #40)
> (In reply to comment #38)
> > >+    m_outputBuffer.AssignLiteral(">");
> > Nit: Could Append('>') instead.
> Why would this be better?
When you assign or append a single character then we don't have to check that the source overlaps the destination. I double-checked and Assign('>') is actually slightly better optimised than Append('>') even if the string is already empty.
Flags: needinfo?(neil)
Ok, thanks.
Attachment #769867 - Attachment is obsolete: true
Attachment #771691 - Flags: review+
Keywords: checkin-needed
Whiteboard: [leave open]
https://hg.mozilla.org/comm-central/rev/67f97998db47
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
Keywords: checkin-needed
Resolution: --- → FIXED
Target Milestone: --- → Thunderbird 25.0
Attachment #771691 - Attachment description: patch for mailnews v4 [to be checked in] → patch for mailnews v4 [checkin: comment 43]
You need to log in before you can comment on or make changes to this bug.