1285262 - (coverity) resource leak: mailnews/imap/src/nsImapServerResponseParser.cpp: |mimeHeaderData| going out of scope leaks the storage it points to.

Status: RESOLVED WORKSFORME

(MailNews Core :: Networking: IMAP, defect)

Description

[ISHIKAWA, Chiaki]

Coverity found this:

Area pointed by |mimeHeaderData| is not freed always. (It is allocated by PR_Malloc()).


2623// mime_header_data should not be streamed out;  rather, it should be
2624// buffered in the nsIMAPBodyShell.
2625// This is because we are still in the process of generating enough
2626// information from the server (such as the MIME header's size) so that

    Test Coverage
    Test Policy Rule
    Exclusions
    Line Impact
    Function Impact

2627// we can construct the final output stream.
2628void nsImapServerResponseParser::mime_header_data()
2629{
2630  char *partNumber = PL_strdup(fNextToken);
    1. Condition partNumber, taking true branch
2631  if (partNumber)
2632  {
2633    char *start = partNumber + 5, *end = partNumber + 5; // 5 == strlen("BODY[")
    2. Condition this->ContinueParse(), taking true branch
    3. Condition end, taking true branch
    4. Condition *end != 'M', taking true branch
    5. Condition *end != 'm', taking true branch
    7. Condition this->ContinueParse(), taking true branch
    8. Condition end, taking true branch
    9. Condition *end != 'M', taking true branch
    10. Condition *end != 'm', taking false branch
2634    while (ContinueParse() && end && *end != 'M' && *end != 'm')
2635    {
2636      end++;
    6. Jumping back to the beginning of the loop
2637    }
    11. Condition end, taking true branch
    12. Condition *end == 'M', taking false branch
    13. Condition *end == 'm', taking true branch
2638    if (end && (*end == 'M' || *end == 'm'))
2639    {
2640      *(end-1) = 0;
2641      AdvanceToNextToken();
    14. alloc_fn: Storage is returned from allocation function CreateAstring. [show details]
    15. var_assign: Assigning: mimeHeaderData = storage returned from this->CreateAstring().
2642      char *mimeHeaderData = CreateAstring();   // is it really this simple?
2643      AdvanceToNextToken();
    16. Condition this->m_shell.operator bool(), taking false branch
2644      if (m_shell)
2645      {
2646        m_shell->AdoptMimeHeader(start, mimeHeaderData);
2647      }
    CID 450529 (#1 of 1): Resource leak (RESOURCE_LEAK)17. leaked_storage: Variable mimeHeaderData going out of scope leaks the storage it points to.
2648    }
2649    else
2650    {
2651      SetSyntaxError(true);
2652    }
2653    PR_Free(partNumber);        // partNumber is not adopted by the body shell.
2654  }
2655  else
2656  {
2657    HandleMemoryFailure();
2658  }
2659}
2660

Observation:

Since |mimeHeaderData| is allocated an area by PR_Malloc(), PR_Free() should release the area.
this-> CreateAstring()  ===> CreateLiteral() ===> PR_Malloc().

Comment 1

[ISHIKAWA, Chiaki]

The following lines were misquoted...

    Test Coverage
    Test Policy Rule
    Exclusions
    Line Impact
    Function Impact

Comment 2

[Wayne Mery (:wsmwk)]

How much are we leaking?

Comment 3

[ISHIKAWA, Chiaki]

It is a variable:

It seems something like this:

The scanner is looking at something like this : "{number"
The pointer is at "{".
The number is scanned to produce the size of the PR_Malloced area.
That is what is lost in 
|mimeHeaderData|.

Caller sequence: 
mimeHeaderData receives a pointer from CreateAstring().

208char *nsIMAPGenericParser::CreateAstring()
209{
   1. Condition *this->fNextToken == '{', taking true branch
210  if (*fNextToken == '{')
   2. alloc_fn: Storage is returned from allocation function CreateLiteral. [show details]
   3. return_alloc_fn: Directly returning storage allocated by CreateLiteral.
211    return CreateLiteral();             // literal
212  else if (*fNextToken == '"')
213    return CreateQuoted();              // quoted
214  else
215    return CreateAtom(true); // atom
216}
217

CreateAstring() returns a pointer from C reateLilteral():

351char *nsIMAPGenericParser::CreateLiteral()
352{
353  int32_t numberOfCharsInMessage = atoi(fNextToken + 1);
354  uint32_t numBytes = numberOfCharsInMessage + 1;
   1. Condition !numBytes, taking false branch
355  NS_ASSERTION(numBytes, "overflow!");
   2. Condition !numBytes, taking false branch
356  if (!numBytes)
357    return nullptr;
   3. alloc_fn: Storage is returned from allocation function PR_Malloc.
   4. var_assign: Assigning: returnString = PR_Malloc(numBytes).
358  char *returnString = (char *)PR_Malloc(numBytes);
   5. Condition !returnString, taking false branch
359  if (!returnString)
360  {
361    HandleMemoryFailure();
362    return nullptr;
363  }
364

Comment 4

[Wayne Mery (:wsmwk)]

Does this leak still show up in coverity?

Comment 5

[Magnus Melin [:mkmelin]]

No coverity says this is gone.