Open Bug 1285434 Opened 4 years ago Updated 7 months ago

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

Categories

(MailNews Core :: Networking: IMAP, defect)

defect
Not set

Tracking

(Not tracked)

People

(Reporter: ishikawa, Unassigned, NeedInfo)

References

(Blocks 2 open bugs, )

Details

(Keywords: coverity, Whiteboard: CID 450306)

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.
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...
potentially major
Severity: normal → major
(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)
Blocks: 1330872
Flags: needinfo?(ishikawa)
See Also: → 1285098
(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.
Flags: needinfo?(ishikawa)
I should not have reset the needinfo flag until I could post something sensible.
Flags: needinfo?(ishikawa)
I am testing a simple patch to release |mailboxName| anyway.
Flags: needinfo?(ishikawa)
Flags: needinfo?(ishikawa)
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);
        }
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
Flags: needinfo?(ishikawa)
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.
Flags: needinfo?(gds)
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?
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.
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.

Setting to normal but keeping my NI.

Severity: major → normal
You need to log in before you can comment on or make changes to this bug.