Bug 23315 - Mismatched new[] / delete in ByteArray
Summary: Mismatched new[] / delete in ByteArray
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: JavaScriptCore (show other bugs)
Version: 528+ (Nightly build)
Hardware: PC OS X 10.5
: P2 Normal
Assignee: Nobody
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2009-01-14 05:43 PST by Dean McNamee
Modified: 2009-01-14 11:08 PST (History)
1 user (show)

See Also:


Attachments
Patch attempt. (1.66 KB, patch)
2009-01-14 10:39 PST, Dean McNamee
darin: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Dean McNamee 2009-01-14 05:43:54 PST
A ByteArray created with ByteArray::create is allocated with new unsigned char[].  There is then a placement new to initialize the ByteArray object on top of this memory.  ByteArray is RefCounted, so it is eventually destroyed with delete.  In theory to do it properly, the destructor for ByteArray should be manually called, and then the memory should be freed with delete[].

My reading of the C++ standard allows new/delete and new[]/delete[] to use completely separate and incompatible allocators, which means it's important that a buffer allocated with new[] is freed with delete[].
Comment 1 Dean McNamee 2009-01-14 05:56:49 PST
One option that might be generally useful is to make RefCounted take a traits, in which Traits::destroy(Type*) is responsible for destroying the object.  The trait would default to something like DefaultRefCountedTrait, which would implement the destroy(Type*) as a delete T;  ByteArray could then supply it's own to do the proper ByteArray::~ByteArray() destructor, and then delete[].

I can try to make this patch, but I imagine any modifications to RefCounted as controversial, so I'll wait for some feedback.

Thanks
Comment 2 Darin Adler 2009-01-14 10:03:31 PST
It's important that new/delete and new[]/delete[] match, but I don't see how an array could be a RefCounted. Let me take a look.
Comment 3 Darin Adler 2009-01-14 10:07:05 PST
OK, I see now.

I don't think we should add traits to RefCounted just so it can be used by ByteArray, though. It doesn't need to inherit from RefCounted. I think it should just inherit from RefCountedBase and implement its own deref function.
Comment 4 David Kilzer (:ddkilzer) 2009-01-14 10:39:14 PST
See also Bug 23123.
Comment 5 Dean McNamee 2009-01-14 10:39:29 PST
Created attachment 26717 [details]
Patch attempt.
Comment 6 Darin Adler 2009-01-14 10:43:00 PST
Comment on attachment 26717 [details]
Patch attempt.

r=me
Comment 7 Oliver Hunt 2009-01-14 10:43:21 PST
Comment on attachment 26717 [details]
Patch attempt.

r=me, do you have commit rights? (i lose track)
Comment 8 Dean McNamee 2009-01-14 10:54:11 PST
I don't, so feel free to commit it for me.  Thanks.
Comment 9 Oliver Hunt 2009-01-14 11:08:05 PST
Committing to http://svn.webkit.org/repository/webkit/trunk ...
	M	JavaScriptCore/ChangeLog
	M	JavaScriptCore/runtime/ByteArray.h
Committed r39897