209808 - Mismatched delete / delete [] in nsCategoryManager.cpp

Status: RESOLVED FIXED

(Core :: XPCOM, defect)

Description

[timeless]

Caught by valgrind the changes for bug 193031 do delete mArray where they should
do delete []mArray
==9397== Mismatched free() / delete / delete []
==9397==    at 0x40047092: __builtin_delete (vg_clientfuncs.c:194)
==9397==    by 0x403EBC32: ???
(/mnt/ibm/mozhack/mozilla/xpcom/components/nsCategoryManager.cpp:101)
==9397==    by 0x403EC682: ???
(/mnt/ibm/mozhack/mozilla/xpcom/components/nsCategoryManager.cpp:188)
==9397==    by 0x403E8CC7: BaseStringEnumerator::Release(void)
(/mnt/ibm/mozhack/mozilla/xpcom/components/nsCate
goryManager.cpp:110)
==9397==    by 0x40394D3B: ??? (../../dist/include/xpcom/nsCOMPtr.h:486)
==9397==    by 0x403EAD50: NS_CreateServicesFromCategory(char const *,
nsISupports *, char const *) (/mnt/ibm/mo
zhack/mozilla/xpcom/components/nsCategoryManager.cpp:794)
==9397==    by 0x403F55BD: nsComponentManagerImpl::AutoRegisterImpl(int, nsIFile
*, int) (/mnt/ibm/mozhack/mozil
la/xpcom/components/nsComponentManager.cpp:3190)
==9397==    by 0x403F6420: nsComponentManagerImpl::AutoRegister(nsIFile *)
(/mnt/ibm/mozhack/mozilla/xpcom/compo
nents/nsComponentManager.cpp:3405)
==9397==    by 0x804C496: main
(/mnt/ibm/mozhack/mozilla/js/src/xpconnect/shell/xpcshell.cpp:902)
==9397==    by 0x4059917D: __libc_start_main (in /lib/libc-2.2.5.so)
==9397==    by 0x804AAB1: (within
/mnt/ibm/mozhack/obj-i686-pc-linux-gnu-qt/js/src/xpconnect/shell/xpcshell)
==9397==    Address 0x41807A80 is 0 bytes inside a block of size 4 alloc'd
==9397==    at 0x40046F28: __builtin_vec_new (vg_clientfuncs.c:156)
==9397==    by 0x403E9026: EntryEnumerator::Create(nsTHashtable<CategoryLeaf> &)
(/mnt/ibm/mozhack/mozilla/xpcom
/components/nsCategoryManager.cpp:186)
==9397==    by 0x403E94EA: CategoryNode::Enumerate(nsISimpleEnumerator **)
(/mnt/ibm/mozhack/mozilla/xpcom/compo
nents/nsCategoryManager.cpp:335)
==9397==    by 0x403EA070: nsCategoryManager::EnumerateCategory(char const *,
nsISimpleEnumerator **) (/mnt/ibm/
mozhack/mozilla/xpcom/components/nsCategoryManager.cpp:611)
==9397==    by 0x403EA7B2: NS_CreateServicesFromCategory(char const *,
nsISupports *, char const *) (/mnt/ibm/mo
zhack/mozilla/xpcom/components/nsCategoryManager.cpp:757)
==9397==    by 0x403F55BD: nsComponentManagerImpl::AutoRegisterImpl(int, nsIFile
*, int) (/mnt/ibm/mozhack/mozil
la/xpcom/components/nsComponentManager.cpp:3190)
==9397==    by 0x403F6420: nsComponentManagerImpl::AutoRegister(nsIFile *)
(/mnt/ibm/mozhack/mozilla/xpcom/compo
nents/nsComponentManager.cpp:3405)
==9397==    by 0x804C496: main
(/mnt/ibm/mozhack/mozilla/js/src/xpconnect/shell/xpcshell.cpp:902)
==9397==    by 0x4059917D: __libc_start_main (in /lib/libc-2.2.5.so)
==9397==    by 0x804AAB1: (within
/mnt/ibm/mozhack/obj-i686-pc-linux-gnu-qt/js/src/xpconnect/shell/xpcshell)

note that delete([]) is null safe so I'm going to remove the if condition too.

Comment 1

[timeless]

Attachment: use delete[]

Comment 2

[Doug Turner (:dougt)]

Comment on attachment 125916 [details] [diff] [review]
use delete[]

delete[] needs to be protected against null.  (delete without the braces
doesn't need to be.  on of the inconsistencies we all love about c++ memory
management)

if that and r=me.

Comment 3

[Alec Flett]

Comment on attachment 125916 [details] [diff] [review]
use delete[]

r/sr=alecf - this is obvious stuff. (and for those reading, yes its safe to
delete[] a null value.

Comment 4

[Alec Flett]

oops, sorry I didn't see dougt's comment. that's pretty silly of C++. Bad language!

Comment 5

[timeless]

err I try to keep track of these things and I was fairly certain that the C++
standard and even all of the compilers we support agree that delete[] 0 is safe.

Anyway, wrt C++ Standard, 5.3.5 #2 says delete[] 0 is safe.
bsmedberg checked an official copy, i checked the only online thing i could
find: http://std.dkuug.dk/jtc1/sc22/open/n2356/expr.html#expr.delete
1 The  delete-expression  operator  destroys  a  most   derived   object
  (_intro.object_) or array created by a new-expression.
          delete-expression:
                  ::opt delete cast-expression
                  ::opt delete [ ] cast-expression
  The ... second is for
  arrays....

2 ...  In either alternative, if the value of the
  operand of delete is the null pointer the operation has no effect....

The c++ portability guidelines don't say anything on this. do we actually know
of any compilers that are so c++98 broken that they do something silly for
delete[] 0?

If we do, then I'll add it to the portability guideline, otherwise I stand by my
memory of the standard and want to remove the if :)

Either way, it's time for me to add a note to the portability guidelines at least on
not mixing
	new[]	w/ delete	(or free) or
	new	w/ delete[]	(or free) or
	malloc	w/ delete or delete[] 
and that
	delete 0 is safe.

That said, i'm going to check this in when the tree opens w/o removing the if
per dougt.

Comment 6

[Doug Turner (:dougt)]

delete [] NULL is definitely not safe for all compilers.  maybe my info is old
and/or the compilers we care about do the right thing in this case.

Comment 7

[Bradley Baetz (:bbaetz)]

yeah, I hvae this vague recolleciton that something didn't like delete[] NULL,
but I also think that that was vacpp, which we no longer support. Not sure on
that, though.

Comment 8

[timeless]

oh well, checked in