From c0e6c668e6e6f0ba6a924a5b83ff1ca5434d14ad Mon Sep 17 00:00:00 2001 From: Trevor Norris Date: Wed, 13 Apr 2016 13:16:42 -0600 Subject: [PATCH] src: no abort from getter if object isn't wrapped v8::Object::GetAlignedPointerFromInternalField() returns a random value if Wrap() hasn't been run on the object handle. Causing v8 to abort if certain getters are accessed. It's possible to access these getters and functions during class construction through the AsyncWrap init() callback, and also possible in a subset of those scenarios while running the persistent handle visitor. Mitigate this issue by manually setting the internal aligned pointer field to nullptr in the BaseObject constructor and add necessary logic to return appropriate values when nullptr is encountered. PR-URL: https://github.com/nodejs/node/pull/6184 Reviewed-By: Ben Noordhuis Reviewed-By: Anna Henningsen --- src/base-object-inl.h | 4 + src/fs_event_wrap.cc | 6 +- src/handle_wrap.cc | 12 +- src/js_stream.cc | 23 +- src/node_contextify.cc | 23 +- src/node_crypto.cc | 276 ++++++++++++++------- src/node_http_parser.cc | 24 +- src/node_internals.h | 2 + src/node_stat_watcher.cc | 6 +- src/node_wrap.h | 2 + src/node_zlib.cc | 15 +- src/pipe_wrap.cc | 18 +- src/process_wrap.cc | 7 +- src/signal_wrap.cc | 6 +- src/stream_base-inl.h | 16 +- src/stream_base.cc | 3 +- src/stream_wrap.cc | 6 +- src/tcp_wrap.cc | 46 +++- src/tls_wrap.cc | 28 ++- src/tty_wrap.cc | 10 +- src/udp_wrap.cc | 29 ++- src/util.h | 20 ++ test/parallel/test-stream-base-no-abort.js | 58 +++++ 23 files changed, 462 insertions(+), 178 deletions(-) create mode 100644 test/parallel/test-stream-base-no-abort.js diff --git a/src/base-object-inl.h b/src/base-object-inl.h index 87159ffc6838c7..86add5e3b37aad 100644 --- a/src/base-object-inl.h +++ b/src/base-object-inl.h @@ -14,6 +14,10 @@ inline BaseObject::BaseObject(Environment* env, v8::Local handle) : handle_(env->isolate(), handle), env_(env) { CHECK_EQ(false, handle.IsEmpty()); + // The zero field holds a pointer to the handle. Immediately set it to + // nullptr in case it's accessed by the user before construction is complete. + if (handle->InternalFieldCount() > 0) + handle->SetAlignedPointerInInternalField(0, nullptr); } diff --git a/src/fs_event_wrap.cc b/src/fs_event_wrap.cc index 48b6f4eca83c4b..3f0df1140c0740 100644 --- a/src/fs_event_wrap.cc +++ b/src/fs_event_wrap.cc @@ -86,7 +86,8 @@ void FSEventWrap::New(const FunctionCallbackInfo& args) { void FSEventWrap::Start(const FunctionCallbackInfo& args) { Environment* env = Environment::GetCurrent(args); - FSEventWrap* wrap = Unwrap(args.Holder()); + FSEventWrap* wrap; + ASSIGN_OR_RETURN_UNWRAP(&wrap, args.Holder()); static const char kErrMsg[] = "filename must be a string or Buffer"; if (args.Length() < 1) @@ -181,7 +182,8 @@ void FSEventWrap::OnEvent(uv_fs_event_t* handle, const char* filename, void FSEventWrap::Close(const FunctionCallbackInfo& args) { - FSEventWrap* wrap = Unwrap(args.Holder()); + FSEventWrap* wrap; + ASSIGN_OR_RETURN_UNWRAP(&wrap, args.Holder()); if (wrap == nullptr || wrap->initialized_ == false) return; diff --git a/src/handle_wrap.cc b/src/handle_wrap.cc index c5792338af2fec..daf821e1d2a197 100644 --- a/src/handle_wrap.cc +++ b/src/handle_wrap.cc @@ -18,7 +18,8 @@ using v8::Value; void HandleWrap::Ref(const FunctionCallbackInfo& args) { - HandleWrap* wrap = Unwrap(args.Holder()); + HandleWrap* wrap; + ASSIGN_OR_RETURN_UNWRAP(&wrap, args.Holder()); if (IsAlive(wrap)) uv_ref(wrap->GetHandle()); @@ -26,7 +27,8 @@ void HandleWrap::Ref(const FunctionCallbackInfo& args) { void HandleWrap::Unref(const FunctionCallbackInfo& args) { - HandleWrap* wrap = Unwrap(args.Holder()); + HandleWrap* wrap; + ASSIGN_OR_RETURN_UNWRAP(&wrap, args.Holder()); if (IsAlive(wrap)) uv_unref(wrap->GetHandle()); @@ -34,7 +36,8 @@ void HandleWrap::Unref(const FunctionCallbackInfo& args) { void HandleWrap::HasRef(const FunctionCallbackInfo& args) { - HandleWrap* wrap = Unwrap(args.Holder()); + HandleWrap* wrap; + ASSIGN_OR_RETURN_UNWRAP(&wrap, args.Holder()); args.GetReturnValue().Set(HasRef(wrap)); } @@ -42,7 +45,8 @@ void HandleWrap::HasRef(const FunctionCallbackInfo& args) { void HandleWrap::Close(const FunctionCallbackInfo& args) { Environment* env = Environment::GetCurrent(args); - HandleWrap* wrap = Unwrap(args.Holder()); + HandleWrap* wrap; + ASSIGN_OR_RETURN_UNWRAP(&wrap, args.Holder()); // Guard against uninitialized handle or double close. if (!IsAlive(wrap)) diff --git a/src/js_stream.cc b/src/js_stream.cc index 6ebdb5a3564136..e51c4ae9b35084 100644 --- a/src/js_stream.cc +++ b/src/js_stream.cc @@ -135,7 +135,8 @@ static void FreeCallback(char* data, void* hint) { void JSStream::DoAlloc(const FunctionCallbackInfo& args) { - JSStream* wrap = Unwrap(args.Holder()); + JSStream* wrap; + ASSIGN_OR_RETURN_UNWRAP(&wrap, args.Holder()); uv_buf_t buf; wrap->OnAlloc(args[0]->Int32Value(), &buf); @@ -150,7 +151,8 @@ void JSStream::DoAlloc(const FunctionCallbackInfo& args) { void JSStream::DoRead(const FunctionCallbackInfo& args) { - JSStream* wrap = Unwrap(args.Holder()); + JSStream* wrap; + ASSIGN_OR_RETURN_UNWRAP(&wrap, args.Holder()); CHECK(Buffer::HasInstance(args[1])); uv_buf_t buf = uv_buf_init(Buffer::Data(args[1]), Buffer::Length(args[1])); @@ -159,8 +161,11 @@ void JSStream::DoRead(const FunctionCallbackInfo& args) { void JSStream::DoAfterWrite(const FunctionCallbackInfo& args) { - JSStream* wrap = Unwrap(args.Holder()); - WriteWrap* w = Unwrap(args[0].As()); + JSStream* wrap; + CHECK(args[0]->IsObject()); + WriteWrap* w; + ASSIGN_OR_RETURN_UNWRAP(&wrap, args.Holder()); + ASSIGN_OR_RETURN_UNWRAP(&w, args[0].As()); wrap->OnAfterWrite(w); } @@ -168,14 +173,17 @@ void JSStream::DoAfterWrite(const FunctionCallbackInfo& args) { template void JSStream::Finish(const FunctionCallbackInfo& args) { - Wrap* w = Unwrap(args[0].As()); + Wrap* w; + CHECK(args[0]->IsObject()); + ASSIGN_OR_RETURN_UNWRAP(&w, args[0].As()); w->Done(args[1]->Int32Value()); } void JSStream::ReadBuffer(const FunctionCallbackInfo& args) { - JSStream* wrap = Unwrap(args.Holder()); + JSStream* wrap; + ASSIGN_OR_RETURN_UNWRAP(&wrap, args.Holder()); CHECK(Buffer::HasInstance(args[0])); char* data = Buffer::Data(args[0]); @@ -197,7 +205,8 @@ void JSStream::ReadBuffer(const FunctionCallbackInfo& args) { void JSStream::EmitEOF(const FunctionCallbackInfo& args) { - JSStream* wrap = Unwrap(args.Holder()); + JSStream* wrap; + ASSIGN_OR_RETURN_UNWRAP(&wrap, args.Holder()); wrap->OnRead(UV_EOF, nullptr); } diff --git a/src/node_contextify.cc b/src/node_contextify.cc index bef7168ada755b..774871b852d021 100644 --- a/src/node_contextify.cc +++ b/src/node_contextify.cc @@ -343,8 +343,8 @@ class ContextifyContext { static void GlobalPropertyGetterCallback( Local property, const PropertyCallbackInfo& args) { - ContextifyContext* ctx = - Unwrap(args.Data().As()); + ContextifyContext* ctx; + ASSIGN_OR_RETURN_UNWRAP(&ctx, args.Data().As()); // Stil initializing if (ctx->context_.IsEmpty()) @@ -373,8 +373,8 @@ class ContextifyContext { Local property, Local value, const PropertyCallbackInfo& args) { - ContextifyContext* ctx = - Unwrap(args.Data().As()); + ContextifyContext* ctx; + ASSIGN_OR_RETURN_UNWRAP(&ctx, args.Data().As()); // Stil initializing if (ctx->context_.IsEmpty()) @@ -387,8 +387,8 @@ class ContextifyContext { static void GlobalPropertyQueryCallback( Local property, const PropertyCallbackInfo& args) { - ContextifyContext* ctx = - Unwrap(args.Data().As()); + ContextifyContext* ctx; + ASSIGN_OR_RETURN_UNWRAP(&ctx, args.Data().As()); // Stil initializing if (ctx->context_.IsEmpty()) @@ -414,8 +414,8 @@ class ContextifyContext { static void GlobalPropertyDeleterCallback( Local property, const PropertyCallbackInfo& args) { - ContextifyContext* ctx = - Unwrap(args.Data().As()); + ContextifyContext* ctx; + ASSIGN_OR_RETURN_UNWRAP(&ctx, args.Data().As()); // Stil initializing if (ctx->context_.IsEmpty()) @@ -430,8 +430,8 @@ class ContextifyContext { static void GlobalPropertyEnumeratorCallback( const PropertyCallbackInfo& args) { - ContextifyContext* ctx = - Unwrap(args.Data().As()); + ContextifyContext* ctx; + ASSIGN_OR_RETURN_UNWRAP(&ctx, args.Data().As()); // Stil initializing if (ctx->context_.IsEmpty()) @@ -806,7 +806,8 @@ class ContextifyScript : public BaseObject { return false; } - ContextifyScript* wrapped_script = Unwrap(args.Holder()); + ContextifyScript* wrapped_script; + ASSIGN_OR_RETURN_UNWRAP(&wrapped_script, args.Holder(), false); Local unbound_script = PersistentToLocal(env->isolate(), wrapped_script->script_); Local