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

Release |body| and |newAttachData| as explained in the original comment.

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.

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