1278948 - (coverity) resource leak: /mailnews/mime/src/mimemoz2.cpp: |attachments| is not released always.

Status: RESOLVED FIXED

(MailNews Core :: MIME, defect)

Description

[ISHIKAWA, Chiaki]

Coverity found this:
I found that, |attachments| can be assigned a new memory area and YET, |MimeGetAttachmentList()| can fail. Thus,
unless we release the newly alloc'ed memory area to |attachments| when |MimeGetAttachmentList()| returns failure code, we do leak memory.


 //
1000    // Ok, now we are going to process the attachment data by getting all
1001    // of the attachment info and then driving the emitter with this data.
1002    //
    5. Condition !msd->options->part_to_load, taking true branch
1003    if (!msd->options->part_to_load || msd->options->format_out == nsMimeOutput::nsMimeMessageBodyDisplay)
1004    {
1005      nsMsgAttachmentData *attachments;
    6. alloc_arg: MimeGetAttachmentList allocates memory that is stored into attachments. [show details]
1006      nsresult rv = MimeGetAttachmentList(obj, msd->url_name, &attachments);
    7. Condition !!!NS_FAILED_impl(rv), taking false branch
    8. Condition !!!NS_FAILED_impl(rv), taking false branch
    9. Condition (bool)!!!NS_FAILED_impl(rv), taking false branch
1007      if (NS_SUCCEEDED(rv))
1008      {
1009        NotifyEmittersOfAttachmentList(msd->options, attachments);
1010        MimeFreeAttachmentList(attachments);
1011      }
    CID 1137494 (#1 of 1): Resource leak (RESOURCE_LEAK)10. leaked_storage: Variable attachments going out of scope leaks the storage it points to.
1012    }
1013
1014    // Release the conversion object - this has to be done after
1015    // we finish processing data.
1016    if ( obj->options)


---
in MimeGetAttachmentList():
we can return failure after a new memory area is alloc'ed attachments. (below nsMsgAttachmentData is |&attachments|.)



   6. alloc_fn: Storage is returned from allocation function operator new[]. [Note: The source code implementation of the function has been overridden by a builtin model.]
   7. var_assign: Assigning: *data = new nsMsgAttachmentData[n + 1].
600  *data = new nsMsgAttachmentData[n + 1];
   8. Condition !*data, taking false branch
601  if (!*data)
602    return NS_ERROR_OUT_OF_MEMORY;
603
604  attIndex = 0;
605
606  // Now, build the list!
607
608  nsresult rv;
609
   9. Condition isAnInlineMessage, taking true branch

610  if (isAnInlineMessage)
600  *data = new nsMsgAttachmentData[n + 1];


   9. Condition isAnInlineMessage, taking true branch
610  if (isAnInlineMessage)
611  {
612    int32_t size = 0;
613    MimeGetSize(obj, &size);
614    rv = GenerateAttachmentData(obj, aMessageURL, obj->options, false, size,
615                                *data);
   10. Condition !!NS_FAILED_impl(__rv), taking true branch
   11. Condition !!NS_FAILED_impl(__rv), taking true branch
   12. Condition (bool)!!NS_FAILED_impl(__rv), taking true branch
616    NS_ENSURE_SUCCESS(rv, rv);
617  }

Comment 1

[ISHIKAWA, Chiaki]

Attachment: Bug 1278948 - release |attachments| by calling |MimeFreeAttachmentList(attachments)| unconditionally.

Proposed patch attached.

Comment 2

[Kent James (:rkent)]

Comment on attachment 8761652 [details] [diff] [review]
Bug 1278948 - release |attachments| by calling |MimeFreeAttachmentList(attachments)| unconditionally.

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

My first expectation is that when an allocating function returns an error, we would assume that the allocation had failed. So I would expect the method with new, should it decide to return an error, would then deallocate. So I would expect the deallocation in MimeGetAttachmentList

Comment 3

[ISHIKAWA, Chiaki]

(In reply to Kent James (:rkent) from comment #2)
> Comment on attachment 8761652 [details] [diff] [review]
> Bug 1278948 - release |attachments| by calling
> |MimeFreeAttachmentList(attachments)| unconditionally.
> 
> Review of attachment 8761652 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> My first expectation is that when an allocating function returns an error,
> we would assume that the allocation had failed. So I would expect the method
> with new, should it decide to return an error, would then deallocate. So I
> would expect the deallocation in MimeGetAttachmentList

So you want a deallocation done in MimeGetAttachmentList when it returns an error.
Let me take a look.

TIA

Comment 4

[ISHIKAWA, Chiaki]

Attachment: Bug 1278948 - release allocated data within MimeGetAttachmentList when it returns failure code

The suggested fix should do it.
It releases the memory allocated within the called function when it exit with an error code.
(And Coverity can check whether this fix is complete or not in the next analysis probably in July.)

TIA

Comment 5

[ISHIKAWA, Chiaki]

Hmm... Maybe I should set |*data| to null after delete.
What do people think ?

Comment 6

[Kent James (:rkent)]

Comment on attachment 8762093 [details] [diff] [review]
Bug 1278948 - release allocated data within MimeGetAttachmentList when it returns failure code

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

This is the right place, but the wrong deallocation.

::: mailnews/mime/src/mimemoz2.cpp
@@ +614,5 @@
>      rv = GenerateAttachmentData(obj, aMessageURL, obj->options, false, size,
>                                  *data);
> +    if (NS_FAILED(rv))
> +    {
> +      delete *data;             // release data in case of error return.

the new statement is an array allocation, so you must use an array de-allocation on this. See MimeGetAttachmentList (though I do not recommend that you use that, which is just confusing syntax sugar).

@@ +622,5 @@
>    }
> +  rv = BuildAttachmentList((MimeObject *) cobj, *data, aMessageURL);
> +  if (NS_FAILED(rv))
> +  {
> +    delete *data;             // release data in case of error return.

Same here.

Comment 7

[ISHIKAWA, Chiaki]

Attachment: Attachments Bug 1278948 - release allocated data within MimeGetAttachmentList when it returns failure code (take 2)

Addressed the issue. 
Now use |delete [] *data] in both places.

FYI: C-C tryserver compilation/test: no new bugs introduced (which means this path is not tested well, I suspect.)

https://treeherder.mozilla.org/#/jobs?repo=try-comm-central&revision=bebe2e0b06ef

Comment 8

[Kent James (:rkent)]

Comment on attachment 8762434 [details] [diff] [review]
Attachments Bug 1278948 - release allocated data within MimeGetAttachmentList when it returns failure code (take 2)

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

Yes looks good now. Thanks!

Comment 9

[ISHIKAWA, Chiaki]

(In reply to Kent James (:rkent) from comment #8)
> Comment on attachment 8762434 [details] [diff] [review]
> Attachments Bug 1278948 - release allocated data within
> MimeGetAttachmentList when it returns failure code (take 2)
> 
> Review of attachment 8762434 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Yes looks good now. Thanks!

Thank you. I will put checkin-needed keyword.

Comment 10

[:aceman]

https://hg.mozilla.org/comm-central/rev/807d3b55d02e91e6091cbe19df272a29aed0ce97