Open Bug 1280273 Opened 4 years ago Updated 4 years ago

(coverity) resource leak: mailnews/mime/src/mimemoz2.cpp: area pointed by |dstPtr| is lost in an error path

Categories

(MailNews Core :: MIME, defect)

defect
Not set

Tracking

(Not tracked)

People

(Reporter: ishikawa, Unassigned)

References

(Blocks 1 open bug, )

Details

(Keywords: coverity, Whiteboard: CID 450347)

Coverity found this:
Area pointed by |dstPtr| is lost in an error path.

 852    rv = encoder->GetMaxLength(unichars, totalChars, &dstLength);
 853    // allocale an output buffer
    12. alloc_fn: Storage is returned from allocation function PR_Malloc.
    13. var_assign: Assigning: dstPtr = storage returned from PR_Malloc(dstLength + 1).
 854    dstPtr = (char *) PR_Malloc(dstLength + 1);
    14. Condition dstPtr == NULL, taking false branch
 855    if (dstPtr == nullptr) {
 856      rv = NS_ERROR_OUT_OF_MEMORY;
 857    }
 858    else {
 859      int32_t buffLength = dstLength;
 860      // convert from unicode
 861      rv = encoder->SetOutputErrorBehavior(nsIUnicodeEncoder::kOnError_Replace, nullptr, '?');
    15. Condition !!!NS_FAILED_impl(rv), taking false branch
    16. Condition !!!NS_FAILED_impl(rv), taking false branch
    17. Condition (bool)!!!NS_FAILED_impl(rv), taking false branch
 862      if (NS_SUCCEEDED(rv)) {
 863        rv = encoder->Convert(unichars, &totalChars, dstPtr, &dstLength);
 864        if (NS_SUCCEEDED(rv)) {
 865          int32_t finLen = buffLength - dstLength;
 866          rv = encoder->Finish((char *)(dstPtr+dstLength), &finLen);
 867          if (NS_SUCCEEDED(rv)) {
 868            dstLength += finLen;
 869          }
 870          dstPtr[dstLength] = '\0';
 871          *pConvertedString = dstPtr;       // set the result string
 872          *outLength = dstLength;
 873        }
 874      }
 875    }
    18. Condition inLength > 144 /* klocalbufsize */, taking true branch
 876    if (inLength > klocalbufsize)
 877      delete [] unichars;
 878  }
 879
    19. Condition !!!NS_FAILED_impl(rv), taking false branch
    20. Condition !!!NS_FAILED_impl(rv), taking false branch
    21. Condition (bool)!!!NS_FAILED_impl(rv), taking false branch
    CID 450347 (#1 of 1): Resource leak (RESOURCE_LEAK)22. leaked_storage: Variable dstPtr going out of scope leaks the storage it points to.
 880  return NS_SUCCEEDED(rv) ? 0 : -1;
 881}
 882

Observation:

In the success path, |dstPtr| is set to |*pConveredString| and thus, it will hopefully be used. So we should not release the area pointed by |dstPtr|.
However, in the false branch (where NS_SUCCEEDED(rv) does not hold on line 862 or 864), |dstPtr| is not set to a global variable or passed argument, and thus will not be used at all... Maybe we should release it in the failure path.

So the possible change might be
 862      if (NS_SUCCEEDED(rv)) {
 863        rv = encoder->Convert(unichars, &totalChars, dstPtr, &dstLength);
 864        if (NS_SUCCEEDED(rv)) {
 865          int32_t finLen = buffLength - dstLength;
 866          rv = encoder->Finish((char *)(dstPtr+dstLength), &finLen);
 867          if (NS_SUCCEEDED(rv)) {
 868            dstLength += finLen;
 869          }
 870          dstPtr[dstLength] = '\0';
 871          *pConvertedString = dstPtr;       // set the result string
 872          *outLength = dstLength;
 873        }
            else
              PR_Free(dstPtr);
 874      }
          else
            PR_Free(dstPtr);
You need to log in before you can comment on or make changes to this bug.