1279249 - (coverity) resource leak: mailnews/mime/src/mimedrft.cpp: |workURLSpec| is not freed always.

Status: RESOLVED FIXED

(MailNews Core :: MIME, defect)

Description

[ISHIKAWA, Chiaki]

Coverity found this:

1717  if (!contLoc && !newAttachment->m_realName.IsEmpty())
    16. alloc_fn: Storage is returned from allocation function ToNewCString.
    17. var_assign: Assigning: workURLSpec = storage returned from ToNewCString(newAttachment->m_realName).
1718    workURLSpec = ToNewCString(newAttachment->m_realName);
    18. Condition contLoc, taking false branch
1719  if ( (contLoc) && (!workURLSpec) )
1720    workURLSpec = strdup(contLoc);
1721
    19. Condition contLoc, taking false branch
1722  PR_FREEIF(contLoc);
1723
1724  mdd->curAttachment = newAttachment;
1725  newAttachment->m_type.Adopt(MimeHeaders_get ( headers, HEADER_CONTENT_TYPE, false, false ));
1726
1727  //
1728  // This is to handle the degenerated Apple Double attachment.
1729  //
1730  parm_value = MimeHeaders_get( headers, HEADER_CONTENT_TYPE, false, false );
    20. Condition parm_value, taking false branch
1731  if (parm_value)
1732  {
1733    char *boundary = NULL;
1734    char *tmp_value = NULL;
1735    boundary = MimeHeaders_get_parameter(parm_value, "boundary", NULL, NULL);
1736    if (boundary)
1737      tmp_value = PR_smprintf("; boundary=\"%s\"", boundary);
1738    if (tmp_value)
1739      newAttachment->m_type = tmp_value;
1740    newAttachment->m_xMacType.Adopt(
1741      MimeHeaders_get_parameter(parm_value, "x-mac-type", NULL, NULL));
1742    newAttachment->m_xMacCreator.Adopt(
1743      MimeHeaders_get_parameter(parm_value, "x-mac-creator", NULL, NULL));
1744    PR_FREEIF(parm_value);
1745    PR_FREEIF(boundary);
1746    PR_FREEIF(tmp_value);
1747  }
1748
1749  newAttachment->m_size = 0;
1750  newAttachment->m_encoding.Adopt(MimeHeaders_get (headers, HEADER_CONTENT_TRANSFER_ENCODING,
1751                                                   false, false));
1752  newAttachment->m_description.Adopt(MimeHeaders_get(headers, HEADER_CONTENT_DESCRIPTION,
1753                                                     false, false ));
1754  //
1755  // If we came up empty for description or the orig URL, we should do something about it.
1756  //
    21. Condition newAttachment->m_description.IsEmpty(), taking false branch
1757  if (newAttachment->m_description.IsEmpty() && workURLSpec)
1758    newAttachment->m_description = workURLSpec;
1759
1760  newAttachment->m_cloudPartInfo.Adopt(MimeHeaders_get(headers,
1761                                       HEADER_X_MOZILLA_CLOUD_PART,
1762                                       false, false));
1763
1764  // There's no file in the message if it's a cloud part.
    22. Condition !newAttachment->m_cloudPartInfo.IsEmpty(), taking true branch
1765  if (!newAttachment->m_cloudPartInfo.IsEmpty())
1766  {
1767    nsAutoCString fileURL;
1768    fileURL.Adopt(
1769      MimeHeaders_get_parameter(newAttachment->m_cloudPartInfo.get(), "file",
1770                                nullptr, nullptr));
    23. Condition !fileURL.IsEmpty(), taking false branch
1771    if (!fileURL.IsEmpty())
1772      nsMimeNewURI(getter_AddRefs(newAttachment->m_origUrl), fileURL.get(),
1773                   nullptr);
1774    mdd->tmpFile = nullptr;
    CID 1137490 (#1 of 1): Resource leak (RESOURCE_LEAK)24. leaked_storage: Variable workURLSpec going out of scope leaks the storage it points to.
1775    return 0;
1776  }

Observation:
|workURLSpec| is released on 1825 with
1825  PR_FREEIF(workURLSpec);
So we should release it before the return in line 1775.

Comment 1

[ISHIKAWA, Chiaki]

Attachment: Bug 1279249 - release |workURLSpec| at early return, too.

Suggested patch.
We simply call PR_FREEIF(workURLSpec); before early return.

Comment 2

[Kent James (:rkent)]

Comment on attachment 8762700 [details] [diff] [review]
Bug 1279249 - release |workURLSpec| at early return, too.

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

Minor change needed.

::: mailnews/mime/src/mimedrft.cpp
@@ +1772,5 @@
>        nsMimeNewURI(getter_AddRefs(newAttachment->m_origUrl), fileURL.get(),
>                     nullptr);
>      mdd->tmpFile = nullptr;
> +
> +    PR_FREEIF(workURLSpec);     // resource leak otherwise

The last place that workURLSpec is used in this file is

newAttachment->m_description = workURLSpec;

near line 1753. That is before the if statement that you are trying to accommodate here. Rather than add another free, it would be better to take the existing free near line 1820 and move it to a point right after the last usage of this variable.

Comment 3

[ISHIKAWA, Chiaki]

Attachment: Release |workURLSpec| always? (take 2)

(In reply to Kent James (:rkent) from comment #2)
> Comment on attachment 8762700 [details] [diff] [review]
> Bug 1279249 - release |workURLSpec| at early return, too.
> 
> Review of attachment 8762700 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Minor change needed.
> 
> ::: mailnews/mime/src/mimedrft.cpp
> @@ +1772,5 @@
> >        nsMimeNewURI(getter_AddRefs(newAttachment->m_origUrl), fileURL.get(),
> >                     nullptr);
> >      mdd->tmpFile = nullptr;
> > +
> > +    PR_FREEIF(workURLSpec);     // resource leak otherwise
> 
> The last place that workURLSpec is used in this file is
> 
> newAttachment->m_description = workURLSpec;
> 
> near line 1753. That is before the if statement that you are trying to
> accommodate here. Rather than add another free, it would be better to take
> the existing free near line 1820 and move it to a point right after the last
> usage of this variable.

I read the comment and checked Coverity's analysis and noticed this:

    21. Condition newAttachment->m_description.IsEmpty(), taking false branch
1757  if (newAttachment->m_description.IsEmpty() && workURLSpec)
1758    newAttachment->m_description = workURLSpec;

*IF* the if-expr on line 1757 is true and workURLSpec is copied to newAttachment->m_description, then we should NOT free workURLSpec because the area pointed by workURLSpec is now referenced through newAttachment->m_description.


It is subtle. So I ended up with the conditional release of |workURLSpec|.

As a matter of fact, it seems releasing |workURLSpec| always is not quite right then. Thus the attached patch.

Comment 4

[Kent James (:rkent)]

"*IF* the if-expr on line 1757 is true and workURLSpec is copied to newAttachment->m_description, then we should NOT free workURLSpec because the area pointed by workURLSpec is now referenced through newAttachment->m_description."

I investigated this issue before I made my recommendation, and I assumed that because m_description is a string class, the string in workURLSpec is copied and therefore not an issue affecting release. Are you able to confirm that?

Comment 5

[ISHIKAWA, Chiaki]

(In reply to Kent James (:rkent) from comment #4)
> "*IF* the if-expr on line 1757 is true and workURLSpec is copied to
> newAttachment->m_description, then we should NOT free workURLSpec because
> the area pointed by workURLSpec is now referenced through
> newAttachment->m_description."
> 
> I investigated this issue before I made my recommendation, and I assumed
> that because m_description is a string class, the string in workURLSpec is
> copied and therefore not an issue affecting release. Are you able to confirm
> that?

Aha. I will check this. Sorry I missed the possibility of assignment doing the COPYing as well. My eyes are so used to reading C code in embedded space over the years. Yes, this is c++ code. Oh well. I will return once I verify the operation.
TIA

Comment 6

[ISHIKAWA, Chiaki]

Attachment: Bug 1279249 - release |workURLSpec| early after the last use to avoid leakBug 1279249 - release |workURLSpec| early after the last use to avoid leak

Yes, the observation that the assignment actually causes the copy of area is correct. (I cheated and checked the generated code instead of checking the headers, etc. to learn what the used types/objects are going to behave.)

Here is the patch.

TIA

Comment 7

[ISHIKAWA, Chiaki]

Attachment: Bug 1279249 - release |workURLSpec| early after the last use to avoid leak PR_FREEIF(workURLSpec);

Thank you again for the review.
Please use this patch. I mixed up the patch with an old one and failed to remove the extra now unnecessary PR_FREEIF.

Comment 8

[Kent James (:rkent)]

Comment on attachment 8763163 [details] [diff] [review]
Bug 1279249 - release |workURLSpec| early after the last use to avoid leak  PR_FREEIF(workURLSpec);

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

LGTM

Comment 9

[aleth [:aleth]]

https://hg.mozilla.org/comm-central/rev/e28ad2d6c0b3da7b4134a8e06acee660bf7630bd
Bug 1279249 - release |workURLSpec| early after the last use to avoid leak. r=rkent