From a3e6208fc7c9488b301ee4f37fb760aadb5cba54 Mon Sep 17 00:00:00 2001 From: Jure Triglav Date: Wed, 13 Dec 2017 22:35:32 +0000 Subject: [PATCH 1/2] src: replace SetAccessor w/ SetAccessorProperty PR-URL: https://github.com/nodejs/node/pull/17665 Fixes: https://github.com/nodejs/node/issues/17636 Refs: https://github.com/nodejs/node/pull/16482 Refs: https://github.com/nodejs/node/pull/16860 Reviewed-By: Anna Henningsen Reviewed-By: Timothy Gu Reviewed-By: James M Snell Reviewed-By: Ben Noordhuis --- src/node_crypto.cc | 81 ++++++++++--------- src/node_crypto.h | 9 +-- src/node_perf.cc | 64 +++++++++++---- src/stream_base-inl.h | 73 +++++++++-------- src/stream_base.h | 9 +-- src/udp_wrap.cc | 23 ++++-- src/udp_wrap.h | 3 +- test/parallel/test-accessor-properties.js | 77 ++++++++++++++++++ .../test-stream-base-prototype-accessors.js | 27 ------- 9 files changed, 231 insertions(+), 135 deletions(-) create mode 100644 test/parallel/test-accessor-properties.js delete mode 100644 test/parallel/test-stream-base-prototype-accessors.js diff --git a/src/node_crypto.cc b/src/node_crypto.cc index ab77993c753..e40942817cd 100644 --- a/src/node_crypto.cc +++ b/src/node_crypto.cc @@ -85,7 +85,6 @@ static const int X509_NAME_FLAGS = ASN1_STRFLGS_ESC_CTRL namespace node { namespace crypto { -using v8::AccessorSignature; using v8::Array; using v8::Boolean; using v8::Context; @@ -108,8 +107,8 @@ using v8::Object; using v8::ObjectTemplate; using v8::Persistent; using v8::PropertyAttribute; -using v8::PropertyCallbackInfo; using v8::ReadOnly; +using v8::Signature; using v8::String; using v8::Value; @@ -402,14 +401,18 @@ void SecureContext::Initialize(Environment* env, Local target) { t->Set(FIXED_ONE_BYTE_STRING(env->isolate(), "kTicketKeyIVIndex"), Integer::NewFromUnsigned(env->isolate(), kTicketKeyIVIndex)); - t->PrototypeTemplate()->SetAccessor( + Local ctx_getter_templ = + FunctionTemplate::New(env->isolate(), + CtxGetter, + env->as_external(), + Signature::New(env->isolate(), t)); + + + t->PrototypeTemplate()->SetAccessorProperty( FIXED_ONE_BYTE_STRING(env->isolate(), "_external"), - CtxGetter, - nullptr, - env->as_external(), - DEFAULT, - static_cast(ReadOnly | DontDelete), - AccessorSignature::New(env->isolate(), t)); + ctx_getter_templ, + Local(), + static_cast(ReadOnly | DontDelete)); target->Set(secureContextString, t->GetFunction()); env->set_secure_context_constructor_template(t); @@ -1335,8 +1338,7 @@ int SecureContext::TicketKeyCallback(SSL* ssl, -void SecureContext::CtxGetter(Local property, - const PropertyCallbackInfo& info) { +void SecureContext::CtxGetter(const FunctionCallbackInfo& info) { SecureContext* sc; ASSIGN_OR_RETURN_UNWRAP(&sc, info.This()); Local ext = External::New(info.GetIsolate(), sc->ctx_); @@ -1406,14 +1408,17 @@ void SSLWrap::AddMethods(Environment* env, Local t) { env->SetProtoMethod(t, "getALPNNegotiatedProtocol", GetALPNNegotiatedProto); env->SetProtoMethod(t, "setALPNProtocols", SetALPNProtocols); - t->PrototypeTemplate()->SetAccessor( + Local ssl_getter_templ = + FunctionTemplate::New(env->isolate(), + SSLGetter, + env->as_external(), + Signature::New(env->isolate(), t)); + + t->PrototypeTemplate()->SetAccessorProperty( FIXED_ONE_BYTE_STRING(env->isolate(), "_external"), - SSLGetter, - nullptr, - env->as_external(), - DEFAULT, - static_cast(ReadOnly | DontDelete), - AccessorSignature::New(env->isolate(), t)); + ssl_getter_templ, + Local(), + static_cast(ReadOnly | DontDelete)); } @@ -2568,8 +2573,7 @@ void SSLWrap::CertCbDone(const FunctionCallbackInfo& args) { template -void SSLWrap::SSLGetter(Local property, - const PropertyCallbackInfo& info) { +void SSLWrap::SSLGetter(const FunctionCallbackInfo& info) { Base* base; ASSIGN_OR_RETURN_UNWRAP(&base, info.This()); SSL* ssl = base->ssl_; @@ -4652,14 +4656,17 @@ void DiffieHellman::Initialize(Environment* env, Local target) { env->SetProtoMethod(t, "setPublicKey", SetPublicKey); env->SetProtoMethod(t, "setPrivateKey", SetPrivateKey); - t->InstanceTemplate()->SetAccessor( + Local verify_error_getter_templ = + FunctionTemplate::New(env->isolate(), + DiffieHellman::VerifyErrorGetter, + env->as_external(), + Signature::New(env->isolate(), t)); + + t->InstanceTemplate()->SetAccessorProperty( env->verify_error_string(), - DiffieHellman::VerifyErrorGetter, - nullptr, - env->as_external(), - DEFAULT, - attributes, - AccessorSignature::New(env->isolate(), t)); + verify_error_getter_templ, + Local(), + attributes); target->Set(FIXED_ONE_BYTE_STRING(env->isolate(), "DiffieHellman"), t->GetFunction()); @@ -4674,14 +4681,17 @@ void DiffieHellman::Initialize(Environment* env, Local target) { env->SetProtoMethod(t2, "getPublicKey", GetPublicKey); env->SetProtoMethod(t2, "getPrivateKey", GetPrivateKey); - t2->InstanceTemplate()->SetAccessor( + Local verify_error_getter_templ2 = + FunctionTemplate::New(env->isolate(), + DiffieHellman::VerifyErrorGetter, + env->as_external(), + Signature::New(env->isolate(), t2)); + + t2->InstanceTemplate()->SetAccessorProperty( env->verify_error_string(), - DiffieHellman::VerifyErrorGetter, - nullptr, - env->as_external(), - DEFAULT, - attributes, - AccessorSignature::New(env->isolate(), t2)); + verify_error_getter_templ2, + Local(), + attributes); target->Set(FIXED_ONE_BYTE_STRING(env->isolate(), "DiffieHellmanGroup"), t2->GetFunction()); @@ -4964,8 +4974,7 @@ void DiffieHellman::SetPrivateKey(const FunctionCallbackInfo& args) { } -void DiffieHellman::VerifyErrorGetter(Local property, - const PropertyCallbackInfo& args) { +void DiffieHellman::VerifyErrorGetter(const FunctionCallbackInfo& args) { HandleScope scope(args.GetIsolate()); DiffieHellman* diffieHellman; diff --git a/src/node_crypto.h b/src/node_crypto.h index 05575da3daa..e2d59e8be5b 100644 --- a/src/node_crypto.h +++ b/src/node_crypto.h @@ -131,8 +131,7 @@ class SecureContext : public BaseObject { const v8::FunctionCallbackInfo& args); static void EnableTicketKeyCallback( const v8::FunctionCallbackInfo& args); - static void CtxGetter(v8::Local property, - const v8::PropertyCallbackInfo& info); + static void CtxGetter(const v8::FunctionCallbackInfo& info); template static void GetCertificate(const v8::FunctionCallbackInfo& args); @@ -290,8 +289,7 @@ class SSLWrap { void* arg); static int TLSExtStatusCallback(SSL* s, void* arg); static int SSLCertCallback(SSL* s, void* arg); - static void SSLGetter(v8::Local property, - const v8::PropertyCallbackInfo& info); + static void SSLGetter(const v8::FunctionCallbackInfo& info); void DestroySSL(); void WaitForCertCb(CertCb cb, void* arg); @@ -676,8 +674,7 @@ class DiffieHellman : public BaseObject { static void SetPublicKey(const v8::FunctionCallbackInfo& args); static void SetPrivateKey(const v8::FunctionCallbackInfo& args); static void VerifyErrorGetter( - v8::Local property, - const v8::PropertyCallbackInfo& args); + const v8::FunctionCallbackInfo& args); DiffieHellman(Environment* env, v8::Local wrap) : BaseObject(env, wrap), diff --git a/src/node_perf.cc b/src/node_perf.cc index 155f1e66e46..e86671d3333 100644 --- a/src/node_perf.cc +++ b/src/node_perf.cc @@ -19,7 +19,7 @@ using v8::Local; using v8::Name; using v8::Object; using v8::ObjectTemplate; -using v8::PropertyCallbackInfo; +using v8::Signature; using v8::String; using v8::Value; @@ -120,8 +120,7 @@ void Measure(const FunctionCallbackInfo& args) { args.GetReturnValue().Set(obj); } -void GetPerformanceEntryName(const Local prop, - const PropertyCallbackInfo& info) { +void GetPerformanceEntryName(const FunctionCallbackInfo& info) { Isolate* isolate = info.GetIsolate(); PerformanceEntry* entry; ASSIGN_OR_RETURN_UNWRAP(&entry, info.Holder()); @@ -129,8 +128,7 @@ void GetPerformanceEntryName(const Local prop, String::NewFromUtf8(isolate, entry->name().c_str(), String::kNormalString)); } -void GetPerformanceEntryType(const Local prop, - const PropertyCallbackInfo& info) { +void GetPerformanceEntryType(const FunctionCallbackInfo& info) { Isolate* isolate = info.GetIsolate(); PerformanceEntry* entry; ASSIGN_OR_RETURN_UNWRAP(&entry, info.Holder()); @@ -138,15 +136,13 @@ void GetPerformanceEntryType(const Local prop, String::NewFromUtf8(isolate, entry->type().c_str(), String::kNormalString)); } -void GetPerformanceEntryStartTime(const Local prop, - const PropertyCallbackInfo& info) { +void GetPerformanceEntryStartTime(const FunctionCallbackInfo& info) { PerformanceEntry* entry; ASSIGN_OR_RETURN_UNWRAP(&entry, info.Holder()); info.GetReturnValue().Set(entry->startTime()); } -void GetPerformanceEntryDuration(const Local prop, - const PropertyCallbackInfo& info) { +void GetPerformanceEntryDuration(const FunctionCallbackInfo& info) { PerformanceEntry* entry; ASSIGN_OR_RETURN_UNWRAP(&entry, info.Holder()); info.GetReturnValue().Set(entry->duration()); @@ -334,14 +330,50 @@ void Init(Local target, Local pe = env->NewFunctionTemplate(PerformanceEntry::New); pe->InstanceTemplate()->SetInternalFieldCount(1); pe->SetClassName(performanceEntryString); + + Local signature = Signature::New(env->isolate(), pe); + + Local get_performance_entry_name_templ = + FunctionTemplate::New(env->isolate(), + GetPerformanceEntryName, + env->as_external(), + signature); + + Local get_performance_entry_type_templ = + FunctionTemplate::New(env->isolate(), + GetPerformanceEntryType, + env->as_external(), + signature); + + Local get_performance_entry_start_time_templ = + FunctionTemplate::New(env->isolate(), + GetPerformanceEntryStartTime, + env->as_external(), + signature); + + Local get_performance_entry_duration_templ = + FunctionTemplate::New(env->isolate(), + GetPerformanceEntryDuration, + env->as_external(), + signature); + Local ot = pe->InstanceTemplate(); - ot->SetAccessor(env->name_string(), GetPerformanceEntryName); - ot->SetAccessor(FIXED_ONE_BYTE_STRING(isolate, "entryType"), - GetPerformanceEntryType); - ot->SetAccessor(FIXED_ONE_BYTE_STRING(isolate, "startTime"), - GetPerformanceEntryStartTime); - ot->SetAccessor(FIXED_ONE_BYTE_STRING(isolate, "duration"), - GetPerformanceEntryDuration); + ot->SetAccessorProperty(env->name_string(), + get_performance_entry_name_templ, + Local()); + + ot->SetAccessorProperty(FIXED_ONE_BYTE_STRING(isolate, "entryType"), + get_performance_entry_type_templ, + Local()); + + ot->SetAccessorProperty(FIXED_ONE_BYTE_STRING(isolate, "startTime"), + get_performance_entry_start_time_templ, + Local()); + + ot->SetAccessorProperty(FIXED_ONE_BYTE_STRING(isolate, "duration"), + get_performance_entry_duration_templ, + Local()); + Local fn = pe->GetFunction(); target->Set(performanceEntryString, fn); env->set_performance_entry_template(fn); diff --git a/src/stream_base-inl.h b/src/stream_base-inl.h index 807e138ef7b..b5d11e3476d 100644 --- a/src/stream_base-inl.h +++ b/src/stream_base-inl.h @@ -11,7 +11,7 @@ namespace node { -using v8::AccessorSignature; +using v8::Signature; using v8::External; using v8::FunctionCallbackInfo; using v8::FunctionTemplate; @@ -34,31 +34,41 @@ void StreamBase::AddMethods(Environment* env, enum PropertyAttribute attributes = static_cast( v8::ReadOnly | v8::DontDelete | v8::DontEnum); - Local signature = - AccessorSignature::New(env->isolate(), t); - t->PrototypeTemplate()->SetAccessor(env->fd_string(), - GetFD, - nullptr, - env->as_external(), - v8::DEFAULT, - attributes, - signature); - - t->PrototypeTemplate()->SetAccessor(env->external_stream_string(), - GetExternal, - nullptr, - env->as_external(), - v8::DEFAULT, - attributes, - signature); - - t->PrototypeTemplate()->SetAccessor(env->bytes_read_string(), - GetBytesRead, - nullptr, - env->as_external(), - v8::DEFAULT, - attributes, - signature); + + Local signature = Signature::New(env->isolate(), t); + + Local get_fd_templ = + FunctionTemplate::New(env->isolate(), + GetFD, + env->as_external(), + signature); + + Local get_external_templ = + FunctionTemplate::New(env->isolate(), + GetExternal, + env->as_external(), + signature); + + Local get_bytes_read_templ = + FunctionTemplate::New(env->isolate(), + GetBytesRead, + env->as_external(), + signature); + + t->PrototypeTemplate()->SetAccessorProperty(env->fd_string(), + get_fd_templ, + Local(), + attributes); + + t->PrototypeTemplate()->SetAccessorProperty(env->external_stream_string(), + get_external_templ, + Local(), + attributes); + + t->PrototypeTemplate()->SetAccessorProperty(env->bytes_read_string(), + get_bytes_read_templ, + Local(), + attributes); env->SetProtoMethod(t, "readStart", JSMethod); env->SetProtoMethod(t, "readStop", JSMethod); @@ -85,8 +95,7 @@ void StreamBase::AddMethods(Environment* env, template -void StreamBase::GetFD(Local key, - const PropertyCallbackInfo& args) { +void StreamBase::GetFD(const FunctionCallbackInfo& args) { // Mimic implementation of StreamBase::GetFD() and UDPWrap::GetFD(). Base* handle; ASSIGN_OR_RETURN_UNWRAP(&handle, @@ -100,10 +109,8 @@ void StreamBase::GetFD(Local key, args.GetReturnValue().Set(wrap->GetFD()); } - template -void StreamBase::GetBytesRead(Local key, - const PropertyCallbackInfo& args) { +void StreamBase::GetBytesRead(const FunctionCallbackInfo& args) { // The handle instance hasn't been set. So no bytes could have been read. Base* handle; ASSIGN_OR_RETURN_UNWRAP(&handle, @@ -115,10 +122,8 @@ void StreamBase::GetBytesRead(Local key, args.GetReturnValue().Set(static_cast(wrap->bytes_read_)); } - template -void StreamBase::GetExternal(Local key, - const PropertyCallbackInfo& args) { +void StreamBase::GetExternal(const FunctionCallbackInfo& args) { Base* handle; ASSIGN_OR_RETURN_UNWRAP(&handle, args.This()); diff --git a/src/stream_base.h b/src/stream_base.h index 20cb0155c9d..7431704fa0c 100644 --- a/src/stream_base.h +++ b/src/stream_base.h @@ -281,16 +281,13 @@ class StreamBase : public StreamResource { int WriteString(const v8::FunctionCallbackInfo& args); template - static void GetFD(v8::Local key, - const v8::PropertyCallbackInfo& args); + static void GetFD(const v8::FunctionCallbackInfo& args); template - static void GetExternal(v8::Local key, - const v8::PropertyCallbackInfo& args); + static void GetExternal(const v8::FunctionCallbackInfo& args); template - static void GetBytesRead(v8::Local key, - const v8::PropertyCallbackInfo& args); + static void GetBytesRead(const v8::FunctionCallbackInfo& args); template target, enum PropertyAttribute attributes = static_cast(v8::ReadOnly | v8::DontDelete); - t->PrototypeTemplate()->SetAccessor(env->fd_string(), - UDPWrap::GetFD, - nullptr, - env->as_external(), - v8::DEFAULT, - attributes); + + Local signature = Signature::New(env->isolate(), t); + + Local get_fd_templ = + FunctionTemplate::New(env->isolate(), + UDPWrap::GetFD, + env->as_external(), + signature); + + t->PrototypeTemplate()->SetAccessorProperty(env->fd_string(), + get_fd_templ, + Local(), + attributes); env->SetProtoMethod(t, "bind", Bind); env->SetProtoMethod(t, "send", Send); @@ -164,7 +171,7 @@ void UDPWrap::New(const FunctionCallbackInfo& args) { } -void UDPWrap::GetFD(Local, const PropertyCallbackInfo& args) { +void UDPWrap::GetFD(const FunctionCallbackInfo& args) { int fd = UV_EBADF; #if !defined(_WIN32) UDPWrap* wrap = Unwrap(args.This()); diff --git a/src/udp_wrap.h b/src/udp_wrap.h index fe9256bcc63..2f4b6a37b5f 100644 --- a/src/udp_wrap.h +++ b/src/udp_wrap.h @@ -38,8 +38,7 @@ class UDPWrap: public HandleWrap { static void Initialize(v8::Local target, v8::Local unused, v8::Local context); - static void GetFD(v8::Local, - const v8::PropertyCallbackInfo&); + static void GetFD(const v8::FunctionCallbackInfo& args); static void New(const v8::FunctionCallbackInfo& args); static void Bind(const v8::FunctionCallbackInfo& args); static void Send(const v8::FunctionCallbackInfo& args); diff --git a/test/parallel/test-accessor-properties.js b/test/parallel/test-accessor-properties.js new file mode 100644 index 00000000000..478b1c55e93 --- /dev/null +++ b/test/parallel/test-accessor-properties.js @@ -0,0 +1,77 @@ +'use strict'; + +require('../common'); + +// This tests that the accessor properties do not raise assertions +// when called with incompatible receivers. + +const assert = require('assert'); + +// Objects that call StreamBase::AddMethods, when setting up +// their prototype +const TTY = process.binding('tty_wrap').TTY; +const UDP = process.binding('udp_wrap').UDP; + +// There are accessor properties in crypto too +const crypto = process.binding('crypto'); + +{ + // Should throw instead of raise assertions + assert.throws(() => { + TTY.prototype.bytesRead; + }, TypeError); + + assert.throws(() => { + TTY.prototype.fd; + }, TypeError); + + assert.throws(() => { + TTY.prototype._externalStream; + }, TypeError); + + assert.throws(() => { + UDP.prototype.fd; + }, TypeError); + + assert.throws(() => { + crypto.SecureContext.prototype._external; + }, TypeError); + + assert.throws(() => { + crypto.Connection.prototype._external; + }, TypeError); + + + // Should not throw for Object.getOwnPropertyDescriptor + assert.strictEqual( + typeof Object.getOwnPropertyDescriptor(TTY.prototype, 'bytesRead'), + 'object' + ); + + assert.strictEqual( + typeof Object.getOwnPropertyDescriptor(TTY.prototype, 'fd'), + 'object' + ); + + assert.strictEqual( + typeof Object.getOwnPropertyDescriptor(TTY.prototype, '_externalStream'), + 'object' + ); + + assert.strictEqual( + typeof Object.getOwnPropertyDescriptor(UDP.prototype, 'fd'), + 'object' + ); + + assert.strictEqual( + typeof Object.getOwnPropertyDescriptor( + crypto.SecureContext.prototype, '_external'), + 'object' + ); + + assert.strictEqual( + typeof Object.getOwnPropertyDescriptor( + crypto.Connection.prototype, '_external'), + 'object' + ); +} diff --git a/test/parallel/test-stream-base-prototype-accessors.js b/test/parallel/test-stream-base-prototype-accessors.js deleted file mode 100644 index f9e12582a09..00000000000 --- a/test/parallel/test-stream-base-prototype-accessors.js +++ /dev/null @@ -1,27 +0,0 @@ -'use strict'; - -require('../common'); - -// This tests that the prototype accessors added by StreamBase::AddMethods -// do not raise assersions when called with incompatible receivers. - -const assert = require('assert'); - -// Or anything that calls StreamBase::AddMethods when setting up its prototype -const TTY = process.binding('tty_wrap').TTY; - -// Should throw instead of raise assertions -{ - const msg = /TypeError: Method \w+ called on incompatible receiver/; - assert.throws(() => { - TTY.prototype.bytesRead; - }, msg); - - assert.throws(() => { - TTY.prototype.fd; - }, msg); - - assert.throws(() => { - TTY.prototype._externalStream; - }, msg); -} From ca3e746da84f248b0a943d00e723ee902b1f1021 Mon Sep 17 00:00:00 2001 From: Rich Trott Date: Tue, 17 Oct 2017 16:59:30 -0700 Subject: [PATCH 2/2] test: make test-tls-external-accessor agnostic MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Remove reliance on V8-specific error messages in test/parallel/test-tls-external-accessor.js. Check that the error is a `TypeError`. The test should now be successful without modification using ChakraCore. Backport-PR-URL: https://github.com/nodejs/node/pull/20456 PR-URL: https://github.com/nodejs/node/pull/16272 Reviewed-By: Michaël Zasso Reviewed-By: Refael Ackermann Reviewed-By: Yuta Hiroto Reviewed-By: Joyee Cheung Reviewed-By: Luigi Pinca Reviewed-By: Colin Ihrig Reviewed-By: James M Snell Reviewed-By: Benjamin Gruenbaum Reviewed-By: Tobias Nießen --- test/parallel/test-tls-external-accessor.js | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/test/parallel/test-tls-external-accessor.js b/test/parallel/test-tls-external-accessor.js index 2d7b1f62b98..33d371923a6 100644 --- a/test/parallel/test-tls-external-accessor.js +++ b/test/parallel/test-tls-external-accessor.js @@ -11,12 +11,12 @@ const tls = require('tls'); { const pctx = tls.createSecureContext().context; const cctx = Object.create(pctx); - assert.throws(() => cctx._external, /incompatible receiver/); + assert.throws(() => cctx._external, TypeError); pctx._external; } { const pctx = tls.createSecurePair().credentials.context; const cctx = Object.create(pctx); - assert.throws(() => cctx._external, /incompatible receiver/); + assert.throws(() => cctx._external, TypeError); pctx._external; }