Closed
Bug 209808
Opened 21 years ago
Closed 21 years ago
Mismatched delete / delete [] in nsCategoryManager.cpp
Categories
(Core :: XPCOM, defect)
Tracking
()
RESOLVED
FIXED
People
(Reporter: timeless, Assigned: timeless)
References
()
Details
Attachments
(1 file)
458 bytes,
patch
|
alecf
:
review+
alecf
:
superreview+
|
Details | Diff | Splinter Review |
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.
Attachment #125916 -
Flags: review?(dougt)
Comment 2•21 years ago
|
||
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•21 years ago
|
||
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.
Attachment #125916 -
Flags: superreview+
Attachment #125916 -
Flags: review?(dougt)
Attachment #125916 -
Flags: review+
Comment 4•21 years ago
|
||
oops, sorry I didn't see dougt's comment. that's pretty silly of C++. Bad language!
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.
Status: NEW → ASSIGNED
Comment 6•21 years ago
|
||
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•21 years ago
|
||
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.
oh well, checked in
Status: ASSIGNED → RESOLVED
Closed: 21 years ago
Resolution: --- → FIXED
You need to log in
before you can comment on or make changes to this bug.
Description
•