1285098 - (coverity) resource leak: mailnews/imap/src/nsImapServerResponseParser.cpp: |boxname| is not released in one execution path

Status: RESOLVED FIXED

(MailNews Core :: Networking: IMAP, defect)

Description

[ISHIKAWA, Chiaki]

Coverity found this:
|boxname| is not released in every path.

 889/* mailbox         ::= "INBOX" / astring
 890*/
 891void nsImapServerResponseParser::mailbox(nsImapMailboxSpec *boxSpec)
 892{
 893  char *boxname = nullptr;
 894  const char *serverKey = fServerConnection.GetImapServerKey();
 895  bool xlistInbox = boxSpec->mBoxFlags & kImapInbox;
 896
    1. Condition !PL_strcasecmp(this->fNextToken, "INBOX"), taking true branch
 897  if (!PL_strcasecmp(fNextToken, "INBOX") || xlistInbox)
 898  {
    2. alloc_fn: Storage is returned from allocation function PL_strdup.
    3. var_assign: Assigning: boxname = storage returned from PL_strdup("INBOX").
 899    boxname = PL_strdup("INBOX");
    4. Condition xlistInbox, taking true branch
 900    if (xlistInbox)
 901      PR_Free(CreateAstring());
 902    AdvanceToNextToken();
    5. Falling through to end of if statement
 903  }
 904  else
 905  {
 906    boxname = CreateAstring();
 907    AdvanceToNextToken();
 908  }
 909
    6. Condition boxname, taking true branch
    7. Condition this->fHostSessionList, taking false branch
 910  if (boxname && fHostSessionList)
 911  {
 912    // should the namespace check go before or after the Utf7 conversion?
 913    fHostSessionList->SetNamespaceHierarchyDelimiterFromMailboxForHost(
 914      serverKey, boxname, boxSpec->mHierarchySeparator);
 915
 916
 917    nsIMAPNamespace *ns = nullptr;
 918    fHostSessionList->GetNamespaceForMailboxForHost(serverKey, boxname, ns);
 919    if (ns)
 920    {
 921      switch (ns->GetType())
 922      {
 923      case kPersonalNamespace:
 924        boxSpec->mBoxFlags |= kPersonalMailbox;
 925        break;
 926      case kPublicNamespace:
 927        boxSpec->mBoxFlags |= kPublicMailbox;
 928        break;
 929      case kOtherUsersNamespace:
 930        boxSpec->mBoxFlags |= kOtherUsersMailbox;
 931        break;
 932      default:  // (kUnknownNamespace)
 933        break;
 934      }
 935      boxSpec->mNamespaceForFolder = ns;
 936    }
 937
 938    //          char *convertedName =
 939    //            fServerConnection.CreateUtf7ConvertedString(boxname, false);
 940    //          char16_t *unicharName;
 941    //        unicharName = fServerConnection.CreatePRUnicharStringFromUTF7(boxname);
 942    //          PL_strfree(boxname);
 943    //          boxname = convertedName;
 944  }
 945
    8. Condition !boxname, taking false branch
 946  if (!boxname)
 947  {
 948    if (!fServerConnection.DeathSignalReceived())
 949      HandleMemoryFailure();
 950  }
    9. Condition boxSpec->mConnection, taking true branch
    10. Condition boxSpec->mConnection->GetCurrentUrl(), taking false branch
 951  else if (boxSpec->mConnection && boxSpec->mConnection->GetCurrentUrl())
 952  {
 953    boxSpec->mConnection->GetCurrentUrl()->AllocateCanonicalPath(boxname, boxSpec->mHierarchySeparator,
 954                                                                 getter_Copies(boxSpec->mAllocatedPathName));
 955    nsIURI *aURL = nullptr;
 956    boxSpec->mConnection->GetCurrentUrl()->QueryInterface(NS_GET_IID(nsIURI), (void **) &aURL);
 957    if (aURL)
 958      aURL->GetHost(boxSpec->mHostName);
 959
 960    NS_IF_RELEASE(aURL);
 961    if (boxname)
 962      PL_strfree( boxname);
 963    // storage for the boxSpec is now owned by server connection
 964    fServerConnection.DiscoverMailboxSpec(boxSpec);
 965
 966    // if this was cancelled by the user,then we sure don't want to
 967    // send more mailboxes their way
 968    if (NS_FAILED(fServerConnection.GetConnectionStatus()))
 969      SetConnected(false);
 970  }
    CID 1137491 (#2-1 of 2): Resource leak (RESOURCE_LEAK)11. leaked_storage: Variable boxname going out of scope leaks the storage it points to.
 971}

Observation:

Maybe we can simply move 
961    if (boxname)
 962      PL_strfree( boxname);
out of else { } and put it at the end of function.

Comment 1

[Wayne Mery (:wsmwk)]

(In reply to ISHIKAWA, Chiaki from comment #0)
> ...
> Observation:
> 
> Maybe we can simply move 
> 961    if (boxname)
>  962      PL_strfree( boxname);
> out of else { } and put it at the end of function.

Sound good to you?

Comment 2

[:aceman]

Can boxname just be nsCString to be safe automatically?

Comment 3

[Magnus Melin [:mkmelin]]

Looks like that would be more refactoring than it's worth.

Comment 4

[Magnus Melin [:mkmelin]]

Attachment: bug1285098_coverty_boxname_leak.patch

Comment 5

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/comm-central/rev/d81799e4dedf
fix memory leak in nsImapServerResponseParser.cpp (coverity issue). r=aceman