1280277 - (coverity) resource leak: mailnews/mime/emitters/nsMimeBaseEmitter.cpp: area pointed by |ptr| is lost in a execution path.

Status: NEW

(MailNews Core :: MIME, defect)

Description

[ISHIKAWA, Chiaki]

Coverity found this:


621  // This is a header so we need to cache and output later.
622  // Ok, now we will setup the header info for the header array!
   5. alloc_fn: Storage is returned from allocation function PR_Calloc.
   6. var_assign: Assigning: ptr = storage returned from PR_Calloc(1U, 16U).
623  headerInfoType  *ptr = (headerInfoType *) PR_NEWZAP(headerInfoType);
   7. Condition ptr, taking true branch
   8. Condition tPtr, taking false branch
624  if ( (ptr) && tPtr)
625  {
626    ptr->name = strdup(field);
627    ptr->value = strdup(value);
628    tPtr->AppendElement(ptr);
629  }
630
   CID 450203 (#1 of 1): Resource leak (RESOURCE_LEAK)9. leaked_storage: Variable ptr going out of scope leaks the storage it points to.
631  return NS_OK;
632}

Observation: this is subtle.
On the surface, it may look that |ptr| is put into the list of |tPtr| and all is OK. But it seems that there is a case of |tPtr| being null (after all it is checked for nullness), and in that case |ptr| is not put into any global reference and thus there is a leak! BUG.
I think we should put something like the following before 631.
if (!tPtr) 
{
   PR_FREEIF(ptr);
}
I also wonder whether we should not return NS_ERROR_UNEXPECTED or somethin in the error case of (!tPtr).
At least we can add NS_ENSURE_STATE(tPtr) or something like that to
catch strange situation in debug build.