Closed Bug 1278948 Opened 4 years ago Closed 4 years ago

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

Categories

(MailNews Core :: MIME, defect)

defect
Not set

Tracking

(Not tracked)

RESOLVED FIXED
Thunderbird 50.0

People

(Reporter: ishikawa, Assigned: ishikawa)

References

(Blocks 1 open bug, )

Details

(Keywords: coverity, Whiteboard: CID 1137494)

Attachments

(1 file, 2 obsolete files)

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 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
(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
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
Attachment #8761652 - Attachment is obsolete: true
Attachment #8762093 - Flags: review?(rkent)
Hmm... Maybe I should set |*data| to null after delete.
What do people think ?
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.
Attachment #8762093 - Flags: review?(rkent) → review-
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
Attachment #8762093 - Attachment is obsolete: true
Attachment #8762434 - Flags: review?(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!
Attachment #8762434 - Flags: review?(rkent) → review+
(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.
Keywords: checkin-needed
https://hg.mozilla.org/comm-central/rev/807d3b55d02e91e6091cbe19df272a29aed0ce97
Status: NEW → RESOLVED
Closed: 4 years ago
Keywords: checkin-needed
OS: Unspecified → All
Hardware: Unspecified → All
Resolution: --- → FIXED
Target Milestone: --- → Thunderbird 50.0
You need to log in before you can comment on or make changes to this bug.