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

Status: RESOLVED FIXED

(MailNews Core :: Composition, defect)

Description

[Geoff Gustafson]

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.

Comment 1

[Scott MacGregor]

putting in the right component.

Comment 2

[Geoff Gustafson]

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.

Comment 3

[scottputterman]

moving to 1.0

Comment 4

[Geoff Gustafson]

*** Bug 103947 has been marked as a duplicate of this bug. ***

Comment 5

[Asa Dotzler [:asa]]

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)

Comment 6

[:aceman]

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.

Comment 7

[Mark Banner (:standard8)]

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.

Comment 8

[:aceman]

OK. I can't yet play these ns*String games.

Comment 9

[Wayne Mery (:wsmwk)]

how much do we leak?  A few bytes?

Comment 10

[:aceman]

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?

Comment 11

[Mark Banner (:standard8)]

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

Comment 12

[:aceman]

Attachment: patch for mime

Comment 13

[:aceman]

Attachment: patch for mailnews

Comment 14

[Joshua Cranmer [:jcranmer]]

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.

Comment 15

[:aceman]

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 16

[Mark Banner (:standard8)]

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.

Comment 17

[Joshua Cranmer [:jcranmer]]

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

[Joshua Cranmer [:jcranmer]]

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.

Comment 19

[[e-mail]]

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)

Comment 20

[:aceman]

I'm not sure why this grew into a reworking of the variables. This may grow the patches 10x.

(In reply to [e-mail] 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?

Comment 21

[:aceman]

Attachment: patch for mime v2

Comment 22

[Joshua Cranmer [:jcranmer]]

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.

Comment 23

[:aceman]

Attachment: patch for mime v3 [checkin: comment 24]

Thanks, done.

Comment 24

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/comm-central/rev/14e6ef96bf28

Comment 25

[Magnus Melin [:mkmelin]]

Attachment: [checked in] external api fix

Truncate(0) -> Truncate()

Comment 26

[[e-mail]]

Comment on attachment 748458 [details] [diff] [review]
[checked in] external api fix

Sorry for the delay, bug 866365 somehow busted my external build.

Comment 27

[Magnus Melin [:mkmelin]]

Comment on attachment 748458 [details] [diff] [review]
[checked in] external api fix

No problem - pushed as https://hg.mozilla.org/comm-central/rev/43c50335022a

Comment 28

[:aceman]

Neil, I'd need reply to comment 20 to continue here.

Comment 29

[[e-mail]]

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

Comment 30

[:aceman]

(In reply to [e-mail] 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.

Comment 31

[:aceman]

Attachment: patch for mailnews v2

Comment 32

[[e-mail]]

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

Comment 33

[:aceman]

(In reply to [e-mail] 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&'

Comment 34

[[e-mail]]

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.

Comment 35

[[e-mail]]

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

Comment 36

[:aceman]

(In reply to [e-mail] 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.

Comment 37

[:aceman]

Attachment: patch for mailnews v3

Comment 38

[[e-mail]]

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.

Comment 39

[:aceman]

(In reply to [e-mail] 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.

Comment 40

[:aceman]

(In reply to [e-mail] from comment #38)
> >+    m_outputBuffer.AssignLiteral(">");
> Nit: Could Append('>') instead.
Why would this be better?

Comment 41

[[e-mail]]

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

Comment 42

[:aceman]

Attachment: patch for mailnews v4 [checkin: comment 43]

Ok, thanks.

Comment 43

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/comm-central/rev/67f97998db47