257385 - various mork functions silently leak in error case

Status: NEW

(MailNews Core :: Database, defect, P5)

Description

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Filing in MailNews: Mail Database for lack of a better component for mork bugs.

Various mork functions, such as:
  morkTableRowCursor::NextRow
  morkTableRowCursor::PrevRow
  morkTableRowCursor::MakeUniqueCursor
  morkTable::GetMetaRow
  morkTable::GetTableRowCursor
  morkTable::PosToRow
  morkTable::NewRow
  morkFactory::CreateNewFile
  morkFactory::MakeHeap
  morkFactory::OpenFilePort
  morkFactory::ThumbToOpenPort
  morkFactory::OpenFileStore
  morkFactory::ThumbToOpenStore
  morkFactory::CreateNewFileStore
and probably others end with code like:

  if ( acqStore )
    *acqStore = outStore;

All of these functions will silently leak if the out parameter is null.

I think these functions should definitely assert if the out parameter is null so
they can't lead to silent leaks.  However, I also don't see any reason to
null-check out parameters (other than for DEBUG-only assertions) when getting
the out parameter is the sole purpose of calling the function -- so I think the
better solution to this bug is to remove the null checks rather than adding
|else outStore->Release();| after each one.

Comment 1

[Wayne Mery (:wsmwk)]

wontfix?

Comment 2

[Masatoshi Kimura [:emk]]

Removing null checks will introduce UB that would be worse than leaks.

Comment 3

[Magnus Melin [:mkmelin]]

What is UB?

Anyway, I suppose we would take a patch to fix it, but given we're trying to get rid of Mork entirely, focusing on that seems like time better spent.

Comment 4

[Wayne Mery (:wsmwk)]

Unpredictable behavior? (just a guess)

Comment 5

[Masatoshi Kimura [:emk]]

UB == undefined behavior.
http://blog.llvm.org/2011/05/what-every-c-programmer-should-know_14.html