From 4394c8a9b3889436fa957ea52955936a2640407b Mon Sep 17 00:00:00 2001 From: Ben Noordhuis Date: Fri, 23 May 2014 15:27:51 +0200 Subject: [PATCH] smalloc: rework double free bug fix Rework the fix from commit 6810132 in a way that removes ~60 lines of code. The bug was introduced in commit e87ceb2 (mea culpa) and is at its core a pointer aliasing bug where sometimes two independent pointers existed that pointed to the same chunk of heap memory. Signed-off-by: Trevor Norris --- src/smalloc.cc | 142 +++++++++++++++---------------------------------- 1 file changed, 42 insertions(+), 100 deletions(-) diff --git a/src/smalloc.cc b/src/smalloc.cc index 918e897813..f2eedc369f 100644 --- a/src/smalloc.cc +++ b/src/smalloc.cc @@ -54,148 +54,93 @@ using v8::WeakCallbackData; using v8::kExternalUnsignedByteArray; -template class CallbackInfo { public: + static inline void Free(char* data, void* hint); static inline CallbackInfo* New(Isolate* isolate, Handle object, - Payload payload); - inline void Dispose(); + FreeCallback callback, + void* hint = 0); + inline void Dispose(Isolate* isolate); inline Persistent* persistent(); - inline Payload* payload(); private: static void WeakCallback(const WeakCallbackData&); + inline void WeakCallback(Isolate* isolate, Local object); inline CallbackInfo(Isolate* isolate, Handle object, - Payload payload); + FreeCallback callback, + void* hint); ~CallbackInfo(); Persistent persistent_; - Payload payload_; -}; - - -class Free { - public: - explicit Free(char* data); - void WeakCallback(Isolate* isolate, - Local object, - CallbackInfo* info); - private: - char* const data_; -}; - - -class Dispose { - public: - typedef void (*Callback)(char* data, void* hint); - Dispose(Callback callback, void* hint); - void WeakCallback(Isolate* isolate, - Local object, - CallbackInfo* info); - private: - Callback const callback_; + FreeCallback const callback_; void* const hint_; + DISALLOW_COPY_AND_ASSIGN(CallbackInfo); }; -template -CallbackInfo* CallbackInfo::New(Isolate* isolate, - Handle object, - Payload payload) { - return new CallbackInfo(isolate, object, payload); +void CallbackInfo::Free(char* data, void*) { + ::free(data); } -template -void CallbackInfo::Dispose() { - delete this; +CallbackInfo* CallbackInfo::New(Isolate* isolate, + Handle object, + FreeCallback callback, + void* hint) { + return new CallbackInfo(isolate, object, callback, hint); } -template -Persistent* CallbackInfo::persistent() { - return &persistent_; +void CallbackInfo::Dispose(Isolate* isolate) { + WeakCallback(isolate, PersistentToLocal(isolate, persistent_)); } -template -Payload* CallbackInfo::payload() { - return &payload_; +Persistent* CallbackInfo::persistent() { + return &persistent_; } -template -CallbackInfo::CallbackInfo(Isolate* isolate, - Handle object, - Payload payload) +CallbackInfo::CallbackInfo(Isolate* isolate, + Handle object, + FreeCallback callback, + void* hint) : persistent_(isolate, object), - payload_(payload) { + callback_(callback), + hint_(hint) { persistent_.SetWeak(this, WeakCallback); persistent_.SetWrapperClassId(ALLOC_ID); persistent_.MarkIndependent(); } -template -CallbackInfo::~CallbackInfo() { +CallbackInfo::~CallbackInfo() { persistent_.Reset(); } -template -void CallbackInfo::WeakCallback( +void CallbackInfo::WeakCallback( const WeakCallbackData& data) { - CallbackInfo* info = data.GetParameter(); - info->payload()->WeakCallback(data.GetIsolate(), data.GetValue(), info); + data.GetParameter()->WeakCallback(data.GetIsolate(), data.GetValue()); } -Free::Free(char* data) : data_(data) { -} - - -void Free::WeakCallback(Isolate* isolate, - Local object, - CallbackInfo* info) { - size_t length = object->GetIndexedPropertiesExternalArrayDataLength(); - if (length > 0) - free(data_); - enum ExternalArrayType array_type = - object->GetIndexedPropertiesExternalArrayDataType(); - size_t array_size = ExternalArraySize(array_type); - CHECK_GT(array_size, 0); - CHECK_GE(array_size * length, length); // Overflow check. - length *= array_size; - int64_t change_in_bytes = -static_cast(length); - isolate->AdjustAmountOfExternalAllocatedMemory(change_in_bytes); - info->Dispose(); -} - - -Dispose::Dispose(Callback callback, void* hint) - : callback_(callback), - hint_(hint) { -} - - -void Dispose::WeakCallback(Isolate* isolate, - Local object, - CallbackInfo* info) { - char* data = - static_cast(object->GetIndexedPropertiesExternalArrayData()); - size_t len = object->GetIndexedPropertiesExternalArrayDataLength(); +void CallbackInfo::WeakCallback(Isolate* isolate, Local object) { + void* array_data = object->GetIndexedPropertiesExternalArrayData(); + size_t array_length = object->GetIndexedPropertiesExternalArrayDataLength(); enum ExternalArrayType array_type = object->GetIndexedPropertiesExternalArrayDataType(); size_t array_size = ExternalArraySize(array_type); CHECK_GT(array_size, 0); if (array_size > 1) { - CHECK_GT(len * array_size, len); // Overflow check. - len *= array_size; + CHECK_GT(array_length * array_size, array_length); // Overflow check. + array_length *= array_size; } - int64_t change_in_bytes = -static_cast(len + sizeof(*info)); + object->SetIndexedPropertiesToExternalArrayData(NULL, array_type, 0); + int64_t change_in_bytes = -static_cast(array_length + sizeof(*this)); isolate->AdjustAmountOfExternalAllocatedMemory(change_in_bytes); - callback_(data, hint_); - info->Dispose(); + callback_(static_cast(array_data), hint_); + delete this; } @@ -403,7 +348,7 @@ void Alloc(Environment* env, env->isolate()->AdjustAmountOfExternalAllocatedMemory(length); size_t size = length / ExternalArraySize(type); obj->SetIndexedPropertiesToExternalArrayData(data, type, size); - CallbackInfo::New(env->isolate(), obj, Free(data)); + CallbackInfo::New(env->isolate(), obj, CallbackInfo::Free); } @@ -421,10 +366,8 @@ void AllocDispose(Environment* env, Handle obj) { Local ext_v = obj->GetHiddenValue(env->smalloc_p_string()); if (ext_v->IsExternal()) { Local ext = ext_v.As(); - CallbackInfo* info = static_cast*>(ext->Value()); - Local object = PersistentToLocal(env->isolate(), - *info->persistent()); - info->payload()->WeakCallback(env->isolate(), object, info); + CallbackInfo* info = static_cast(ext->Value()); + info->Dispose(env->isolate()); return; } } @@ -484,8 +427,7 @@ void Alloc(Environment* env, Isolate* isolate = env->isolate(); HandleScope handle_scope(isolate); env->set_using_smalloc_alloc_cb(true); - CallbackInfo* info = - CallbackInfo::New(isolate, obj, Dispose(fn, hint)); + CallbackInfo* info = CallbackInfo::New(isolate, obj, fn, hint); obj->SetHiddenValue(env->smalloc_p_string(), External::New(isolate, info)); isolate->AdjustAmountOfExternalAllocatedMemory(length + sizeof(*info)); size_t size = length / ExternalArraySize(type);