From 12300626d705d7e07f07bec5885d56518ccc97a3 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 | 7 +- src/handle_wrap.cc | 9 +- src/js_stream.cc | 23 +- src/node_contextify.cc | 23 +- src/node_crypto.cc | 265 ++++++++++++++------- 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 | 14 ++ test/parallel/test-stream-base-no-abort.js | 58 +++++ 23 files changed, 446 insertions(+), 175 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 db0daa1e82f559..e31435af07285a 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 9d96cd51a24cee..7d1ff18da7449f 100644 --- a/src/fs_event_wrap.cc +++ b/src/fs_event_wrap.cc @@ -84,8 +84,8 @@ void FSEventWrap::New(const FunctionCallbackInfo& args) { void FSEventWrap::Start(const FunctionCallbackInfo& args) { Environment* env = Environment::GetCurrent(args); - FSEventWrap* wrap = Unwrap(args.Holder()); - CHECK_EQ(wrap->initialized_, false); + FSEventWrap* wrap; + ASSIGN_OR_RETURN_UNWRAP(&wrap, args.Holder()); if (args.Length() < 1 || !args[0]->IsString()) { return env->ThrowTypeError("filename must be a valid string"); @@ -165,7 +165,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 3fc33cbd5bb58f..318300c2540a83 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->handle__); @@ -28,7 +29,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->handle__); @@ -40,7 +42,8 @@ void HandleWrap::Unref(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 7404bbba87ec45..d05f8e8f0e6baf 100644 --- a/src/node_contextify.cc +++ b/src/node_contextify.cc @@ -340,8 +340,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()) @@ -370,8 +370,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()) @@ -384,8 +384,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()) @@ -411,8 +411,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()) @@ -427,8 +427,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()) @@ -690,7 +690,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