1285434 - (coverity) resource leak: mailnews/imap/src/nsImapServerResponseParser.cpp: |mailboxName| is not released.

Status: RESOLVED WONTFIX

(MailNews Core :: Networking: IMAP, defect)

Description

[ISHIKAWA, Chiaki]

Coverity found this:
|mailboxName| is not released.

2319void nsImapServerResponseParser::xmailboxinfo_data()
2320{
2321  AdvanceToNextToken();
    1. Condition !this->fNextToken, taking false branch
2322  if (!fNextToken)
2323    return;
2324
    2. alloc_fn: Storage is returned from allocation function CreateAstring. [show details]
    3. var_assign: Assigning: mailboxName = storage returned from this->CreateAstring().
2325  char *mailboxName = CreateAstring(); // PL_strdup(fNextToken);
    4. Condition mailboxName, taking true branch
2326  if (mailboxName)
2327  {
2328    do
2329    {
2330      AdvanceToNextToken();
    5. Condition this->fNextToken, taking true branch
2331      if (fNextToken)
2332      {
    6. Condition !PL_strcmp("MANAGEURL", this->fNextToken), taking true branch
2333        if (!PL_strcmp("MANAGEURL", fNextToken))
2334        {
2335          AdvanceToNextToken();
2336          fFolderAdminUrl = CreateAstring();
    7. Falling through to end of if statement
2337        }
2338        else if (!PL_strcmp("POSTURL", fNextToken))
2339        {
2340          AdvanceToNextToken();
2341          // ignore this for now...
2342        }
2343      }
    8. Condition this->fNextToken, taking true branch
    9. Condition !this->fAtEndOfLine, taking false branch
2344    } while (fNextToken && !fAtEndOfLine && ContinueParse());
2345  }
    CID 450306 (#1 of 1): Resource leak (RESOURCE_LEAK)10. leaked_storage: Variable mailboxName going out of scope leaks the storage it points to.
2346}
2347

Observation: a simple bug.
I wish all the issues reported are this simple.

Comment 1

[ISHIKAWA, Chiaki]

Well, on second look, in the unlikely case of failure to allocate mailboxName (= null), the parsing never proceeds past a point. This is possibly a more serious bug because TB will look to be stuck in a loop. Hmm...

Comment 2

[Wayne Mery (:wsmwk)]

potentially major

Comment 3

[Wayne Mery (:wsmwk)]

(In reply to ISHIKAWA, Chiaki from comment #1)
> Well, on second look, in the unlikely case of failure to allocate
> mailboxName (= null), the parsing never proceeds past a point. This is
> possibly a more serious bug because TB will look to be stuck in a loop.
> Hmm...

Can we tackle this one?
What do we need to make progress?

(Asking because we need to get a grip on our more serious leaks, which seem to have gotten worse in 52.x)

Comment 4

[ISHIKAWA, Chiaki]

(In reply to Wayne Mery (:wsmwk, NI for questions) from comment #3)
> (In reply to ISHIKAWA, Chiaki from comment #1)
> > Well, on second look, in the unlikely case of failure to allocate
> > mailboxName (= null), the parsing never proceeds past a point. This is
> > possibly a more serious bug because TB will look to be stuck in a loop.
> > Hmm...
> 
> Can we tackle this one?
> What do we need to make progress?
> 
> (Asking because we need to get a grip on our more serious leaks, which seem
> to have gotten worse in 52.x)

I will take a look soonish. Got sidetracked by a sudden peak of day job workload.

Comment 5

[ISHIKAWA, Chiaki]

I should not have reset the needinfo flag until I could post something sensible.

Comment 6

[ISHIKAWA, Chiaki]

I am testing a simple patch to release |mailboxName| anyway.

Comment 7

[Jorg K (CEST = GMT+2)]

nsImapServerResponseParser.cpp is a can of worms. Check the use of CreateAstring() which returns an allocated string. As far as I can tell, that can leak on more occasions than reported here.

Looks like nsImapServerResponseParser::xmailboxinfo_data() always leaks 'mailboxName'. Equally nsImapServerResponseParser::xmailboxinfo_data() always leaks 'fFolderAdminUrl'.

nsImapServerResponseParser::mime_header_data() can leak 'mimeHeaderData' if 'm_shell' is null and the thing isn't adopted.

BTW, does anyone understand this?
        if (fNextToken)
        {
          char *mailboxName = CreateAstring();
          PL_strfree( mailboxName);
        }

Comment 8

[ISHIKAWA, Chiaki]

OK, I have been occupied with day job and family stuff, but I will take a look.
I hate to say this, but IMAP-related code seems to be full of worms (as Jorg K stated about a file there), and I have stayed away from it until now.
Now that I will use more IMAP servers (due to office mail admin policy, etc.), I have to bite the bullet and get on.

TIA

Comment 9

[Wayne Mery (:wsmwk)]

a can of worms is provided so perhaps Gene will be interested in fishing? :)

(lots of good info here)

other leaks
Bug 1285262 - (coverity) resource leak: mailnews/imap/src/nsImapServerResponseParser.cpp:  |mimeHeaderData| going out of scope leaks the storage it points to.
Bug 1285432 - (coverity) resource leak: mailnews/imap/src/nsIMAPNamespace.cpp: |convertedFolderName| is not released in an early error path.

Comment 10

[gene smith]

I'm not familiar with "coverity". I see it's a static analysis program. Does it run as part of the tb build or is it just something that is being run by the reporter on his own?

Comment 11

[Jorg K (CEST = GMT+2)]

Someone runs it on the Mozilla code base. All you need to do is get there report and look at the code it relates to. These seem to be leaks, so something that gets allocated but not released on all code paths. Usually easy wins unless it's spaghetti code.

Comment 12

[Wayne Mery (:wsmwk)]

Coverity scans and reports are not part of our build system nor run manually.  They are automatic, listed at https://scan.coverity.com/dashboard,  although there may have been a recent lapse. The last analyzed date is Apr 22, 2018. I've emailed an admin to get them running again.  

Ishikawa and I file bugs as appropriate from the coverity reports that are emailed to us.

Comment 13

[gene smith]

Setting to normal but keeping my NI.

Comment 14

[Wayne Mery (:wsmwk)]

With imap-js on the horizon in the next year, no data on the extent of the leak, and no progress in many years, let's declare this wontfix.
Unless Gene has a safe fix in his back pocket.