Open Bug 1279538 Opened 4 years ago Updated 4 years ago

(coverity) resouce leak : mailnews/mime/src/mimedrft.cpp: |body| and |newAttachDAta| are not released.

Categories

(MailNews Core :: MIME, defect)

defect
Not set

Tracking

(Not tracked)

People

(Reporter: ishikawa, Assigned: ishikawa)

References

(Blocks 1 open bug, )

Details

(Keywords: coverity, Whiteboard: CID 1137488 CID 1137489)

Attachments

(1 file)

Coverity found this:
In the early return case, |body| and |newAttachData| are not released.
(Coverity User-Interface has a feature that lists only one such target of resource leaks and we can see different target by toggling |[select issue]| on the UI, but that feature is not very useful for reporting the issue by copy&paste to bugzilla.
I also noticed that this is the file that gets reported by bug 1279249 for another resource leak. I am filing bugzilla per Coverity issue unless the same line (function exit) causes resouce leaks of a few targets. Coverity lists each of the target resource in a different report entry. CID 1137488 for |body| and CID 1138489 for |newAttachData|.



1339        bodyLen = fileSize;
1340        body = (char *)PR_MALLOC (bodyLen + 1);
    21. Condition body, taking true branch
1341        if (body)
1342        {
1343          memset (body, 0, bodyLen+1);
1344
1345          uint32_t bytesRead;
1346          nsCOMPtr <nsIInputStream> inputStream;
1347
1348          nsresult rv = NS_NewLocalFileInputStream(getter_AddRefs(inputStream), mdd->messageBody->m_tmpFile);
    22. Condition !!NS_FAILED_impl(rv), taking true branch
    23. Condition !!NS_FAILED_impl(rv), taking true branch
    24. Condition (bool)!!NS_FAILED_impl(rv), taking true branch
1349          if (NS_FAILED(rv))
    CID 1137488: Resource leak (RESOURCE_LEAK) [select issue]
    CID 1137489 (#1 of 1): Resource leak (RESOURCE_LEAK)25. leaked_storage: Variable newAttachData going out of scope leaks the storage it points to.
1350            return;
1351

Observation:
In the same file, |body| is released eventually by
 // convert from UTF-8 to UTF-16 
1444 if (body) 
1445 { 
1446    fields->SetBody(NS_ConvertUTF8toUTF16(body)); 
1447    PR_Free(body); 
1448 } 
1449
So PR_Free(body) should do it.

For |newAttachData|, it is relased in normal processing as follows.
1581 delete [] newAttachData;

So |delete| will do it.


1352
See Also: → 1279249
Release |body| and |newAttachData|
as explained in the original comment.
Assignee: nobody → ishikawa
Attachment #8763123 - Flags: review?(rkent)
Comment on attachment 8763123 [details] [diff] [review]
Bug 1279538 - release |body| and |newAttachData| in early return case, too.

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

The issue I have with this is that it does not go far enough. This method has a whole bunch of freeing needed before it returns, see the code from "if ( mdd->headers ) MimeHeaders_free ( mdd->headers );" to the end of the method. I'm not sure that it makes sense to free a couple of areas without making the error return clean with respect to the other variables as well.

Yes "goto statement considered harmful" but this is a case where setting an rv error value, then jumping to the exit portion of the code, is probably the best way to handle this. Other methods here use a jump to FAIL:  as the solution, you should probably follow the same pattern.

All of the PR_FREEIF of local variables are no-brainers as OK. Not sure of the mdd-> references, perhaps you could comment on those.
Attachment #8763123 - Flags: review?(rkent) → review-
> 
> The issue I have with this is that it does not go far enough. This method
> has a whole bunch of freeing needed before it returns, see the code from "if
> ( mdd->headers ) MimeHeaders_free ( mdd->headers );" to the end of the
> method. I'm not sure that it makes sense to free a couple of areas without
> making the error return clean with respect to the other variables as well.
> 
> Yes "goto statement considered harmful" but this is a case where setting an
> rv error value, then jumping to the exit portion of the code, is probably
> the best way to handle this. Other methods here use a jump to FAIL:  as the
> solution, you should probably follow the same pattern.
> 
> All of the PR_FREEIF of local variables are no-brainers as OK. Not sure of
> the mdd-> references, perhaps you could comment on those.

Your points are well taken.

Regarding my coverity reporting, and suggested fix, I will look only locally (due to lack of time, sorry) to come up with a local solution (unless I am motivated enough to re-arrange a few things, etc.)
In this suggested patch, I was looking very locally at the spot where the problem occurred.
Actually, I noticed the big block of PR_FREEIF()'s, but I was not sure
whether other variables were assigned values at the time of early return. 
Now I realize they are given 0's as initial value after reading your comment. 

I am not opposed to use of "goto" for error handling with resource release requirements (!)
I am glad to find a sympathetic voice: I think using goto is fine as long as
 - only jumping forward at the end of the function/procedure for error handling purposes,
 - the label should be one of "SUCCESS", "FAILURE" ("ERROR" or "EXIT"), EXIT_FREE, EXIT_FREE_1, EXIT_FREE_2, EXIT_FREE_3, etc. (or ERROR_FREE, ERROR_FREE_1, ERROR_FREE_2, etc.)

The order of appearances of the labels ought to be
EXIT_FREE_N: /* highest */
...
EXIT_FREE_1:
...
EXIT_FREE:

SUCCESS can come either before the EXIT (or ERROR) or after, but I suspect for resource deallocation purposes
maybe it should come AFTER all the EXIT (or ERROR) labels.
So I will take advantage of the big block consisting of PR_FREEIF()s at the end.

One thing that surprises me is that there is only one place where the early return is done
and use of goto EXIT_FREE is required now. Only in one place(!?)
I looked at the code and found the failure of PR_MALLOC() is silently ignored (or is it infalllible?)
I think the failure ought to invoke this goto EXIT_FREE.
So I inserted a few such instances. In a few places, the failure to allocate a new buffer only result in a unconverted (in terms of char code, etc.) to be used, and I let it pass.

BTW, I don't like the size of this BIG function.
It is 492 lines of code. 
I think anything over 150 lines is a suspect for maintainability in the long run:  especially if there is no accompanying documentation or well written comment.
This function has neither, I think.
But I dare not break this function up at this stage.

I wish we have more man-power to evaluate coverity issues, but I should not complain. I saw firefox has 2.5 K bugs reported by it. TB has about 1/10 of the bugs (excluding ldap-related issues.)

BTW, Near line 1156, we have a comment:
     // mscott: aren't we leaking a bunch of strings here like the charset strings and such?
I silently ignore this until Coverity or some other tools (or persistent human developer) raises this question again with some valid argument as Coverity shows in its analysis.
Except, I tried to add 
      PR_FREEIF(mdd->options->url); // CI
at least, after checking the definition of mdd->options.
  
Please wait for a couple of days.
I found out that fixing this function along your suggestion is not easy :-(
[The current patch is the easiest route to address the issue Coverity reported.]
Ahem, interesting find.

|body| is declared inside a surrounding block where the early exit is taken.
Thus |body| needs to be released there, and NOT at the end of the function.
So we have to PR_Free(body) in the early error path anyway. 
This makes the approach of using |goto| a little unattractive. 

On a brighter front, I noticed by re-arranging the list of PR_FREEIF()'s at the end of the function so that they appear in the reverse order of
allocations that
three local variables that appear in PR_FREEIF() are NOT USED AT ALL!
So I can remove at least six lines (three declarations, three PR_FREEIF()'s).

Stay tuned.
You need to log in before you can comment on or make changes to this bug.