From 356a143cf93e9bbd6c0f6966d14160cf86da19f2 Mon Sep 17 00:00:00 2001 From: Anna Henningsen Date: Mon, 25 Feb 2019 02:32:51 +0100 Subject: [PATCH 1/4] build: remove v8_typed_array_max_size_in_heap option MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit This was added in 16f86d6c578ff7aec708c7d736558a199d290e9c, based on the assumption that otherwise, the memory behind `ArrayBuffer` instances could be moved around on the heap while native code holds references to it. This does not match what V8 actually does (and also did at the time): - The option/build variable was about always only about TypedArrays, not ArrayBuffers. Calls like `new ArrayBuffer(4)` call into C++ regardless of the option value, but calls like `new Uint8Array(4)` would not call into C++ under V8 defaults. - When first accessing a heap-allocated TypedArray’s `ArrayBuffer`, whether that is through the JS `.buffer` getter or the C++ `ArrayBufferView::Buffer()` function, a copy of the contents is created using the ArrayBuffer allocator and stored as the (permanent, unmovable) backing store. As a consequence, the memory returned by `ArrayBuffer::GetContents()` is not moved around, because it is fixed once the `ArrayBuffer` object itself first comes into explicit existence in any way. Removing this build option significantly speeds up creation of typed arrays from JS: $ ./node benchmark/compare.js --new ./node --old ./node-master --runs 10 --filter buffer-creation.js buffers | Rscript benchmark/compare.R confidence improvement accuracy (*) (**) (***) buffers/buffer-creation.js n=1024 len=10 type='buffer()' *** 593.66 % ±28.64% ±41.10% ±60.36% buffers/buffer-creation.js n=1024 len=10 type='fast-alloc-fill' *** 675.42 % ±90.67% ±130.24% ±191.54% buffers/buffer-creation.js n=1024 len=10 type='fast-alloc' *** 663.55 % ±58.41% ±83.87% ±123.29% buffers/buffer-creation.js n=1024 len=10 type='fast-allocUnsafe' 3.10 % ±9.63% ±13.22% ±18.07% buffers/buffer-creation.js n=1024 len=10 type='slow-allocUnsafe' 4.67 % ±5.55% ±7.77% ±10.97% buffers/buffer-creation.js n=1024 len=10 type='slow' -2.48 % ±4.47% ±6.12% ±8.34% buffers/buffer-creation.js n=1024 len=1024 type='buffer()' -1.91 % ±4.71% ±6.45% ±8.79% buffers/buffer-creation.js n=1024 len=1024 type='fast-alloc-fill' -1.34 % ±7.53% ±10.33% ±14.10% buffers/buffer-creation.js n=1024 len=1024 type='fast-alloc' 0.52 % ±5.00% ±6.87% ±9.40% buffers/buffer-creation.js n=1024 len=1024 type='fast-allocUnsafe' 0.39 % ±5.65% ±7.78% ±10.67% buffers/buffer-creation.js n=1024 len=1024 type='slow-allocUnsafe' -0.13 % ±5.68% ±7.83% ±10.77% buffers/buffer-creation.js n=1024 len=1024 type='slow' -5.07 % ±7.15% ±9.80% ±13.35% buffers/buffer-creation.js n=1024 len=2048 type='buffer()' 0.57 % ±2.70% ±3.74% ±5.16% buffers/buffer-creation.js n=1024 len=2048 type='fast-alloc-fill' -1.60 % ±4.96% ±6.79% ±9.25% buffers/buffer-creation.js n=1024 len=2048 type='fast-alloc' 1.29 % ±3.79% ±5.20% ±7.09% buffers/buffer-creation.js n=1024 len=2048 type='fast-allocUnsafe' 2.73 % ±8.79% ±12.05% ±16.41% buffers/buffer-creation.js n=1024 len=2048 type='slow-allocUnsafe' -0.99 % ±6.27% ±8.65% ±11.91% buffers/buffer-creation.js n=1024 len=2048 type='slow' -5.98 % ±6.24% ±8.71% ±12.20% buffers/buffer-creation.js n=1024 len=4096 type='buffer()' -1.75 % ±3.48% ±4.78% ±6.56% buffers/buffer-creation.js n=1024 len=4096 type='fast-alloc-fill' -3.18 % ±3.97% ±5.45% ±7.45% buffers/buffer-creation.js n=1024 len=4096 type='fast-alloc' 2.05 % ±4.05% ±5.58% ±7.65% buffers/buffer-creation.js n=1024 len=4096 type='fast-allocUnsafe' 1.44 % ±5.51% ±7.63% ±10.57% buffers/buffer-creation.js n=1024 len=4096 type='slow-allocUnsafe' * -4.77 % ±4.30% ±5.90% ±8.06% buffers/buffer-creation.js n=1024 len=4096 type='slow' -3.31 % ±6.38% ±8.86% ±12.34% buffers/buffer-creation.js n=1024 len=8192 type='buffer()' 0.06 % ±2.70% ±3.77% ±5.31% buffers/buffer-creation.js n=1024 len=8192 type='fast-alloc-fill' -1.20 % ±3.30% ±4.53% ±6.17% buffers/buffer-creation.js n=1024 len=8192 type='fast-alloc' -1.46 % ±2.75% ±3.84% ±5.38% buffers/buffer-creation.js n=1024 len=8192 type='fast-allocUnsafe' 1.27 % ±4.69% ±6.49% ±8.98% buffers/buffer-creation.js n=1024 len=8192 type='slow-allocUnsafe' -1.68 % ±3.30% ±4.62% ±6.49% buffers/buffer-creation.js n=1024 len=8192 type='slow' -2.49 % ±3.24% ±4.44% ±6.07% (Re-running the outlier with 30 runs instead of 10:) buffers/buffer-creation.js n=1024 len=4096 type='slow-allocUnsafe' 2.06 % ±2.39% ±3.19% ±4.15% The performance gains effect are undone once native code accesses the underlying ArrayBuffer, but then again that a) does not happen for all TypedArrays, and b) it should also make sense to look into using `ArrayBufferView::CopyContents()` in some places, which is made specifically to avoid such a performance impact and allows us to use the benefits of heap-allocated typed arrays. Refs: https://github.com/nodejs/node/commit/16f86d6c578ff7aec708c7d736558a199d290e9c Refs: https://github.com/nodejs/node/pull/2893 Refs: https://github.com/nodejs/node/commit/74178a5682958d499896e0fa1af6bc0321ec1935#commitcomment-13250880 Refs: http://logs.libuv.org/node-dev/2015-09-15 --- configure.py | 4 ---- 1 file changed, 4 deletions(-) diff --git a/configure.py b/configure.py index a3f885c0fd0d8f..ac6b3d4788cd1b 100755 --- a/configure.py +++ b/configure.py @@ -1123,10 +1123,6 @@ def configure_v8(o): o['variables']['node_use_bundled_v8'] = b(not options.without_bundled_v8) o['variables']['force_dynamic_crt'] = 1 if options.shared else 0 o['variables']['node_enable_d8'] = b(options.enable_d8) - # Unconditionally force typed arrays to allocate outside the v8 heap. This - # is to prevent memory pointers from being moved around that are returned by - # Buffer::Data(). - o['variables']['v8_typed_array_max_size_in_heap'] = 0 if options.enable_d8: o['variables']['test_isolation_mode'] = 'noop' # Needed by d8.gyp. if options.without_bundled_v8 and options.enable_d8: From 250dce31507fdc7afcee2ed8707128ad0402c6c6 Mon Sep 17 00:00:00 2001 From: Anna Henningsen Date: Mon, 25 Feb 2019 04:12:19 +0100 Subject: [PATCH 2/4] src: allow not materializing ArrayBuffers from C++ Where appropriate, use a helper that wraps around `ArrayBufferView::Buffer()` or `ArrayBufferView::CopyContents()` rather than `Buffer::Data()`, as that may help to avoid materializing the underlying `ArrayBuffer` when reading small typed arrays from C++. This allows keeping the performance benefits of the faster creation of heap-allocated small typed arrays in many cases. --- src/node_crypto.cc | 227 ++++++++++++++++++++---------------------- src/node_crypto.h | 7 +- src/node_http2.cc | 26 +++-- src/node_http2.h | 5 +- src/string_decoder.cc | 9 +- src/util-inl.h | 26 +++++ src/util.cc | 7 +- src/util.h | 21 ++++ 8 files changed, 183 insertions(+), 145 deletions(-) diff --git a/src/node_crypto.cc b/src/node_crypto.cc index 49b9dfe576c060..fbd5866cd45d1b 100644 --- a/src/node_crypto.cc +++ b/src/node_crypto.cc @@ -57,6 +57,7 @@ namespace crypto { using node::THROW_ERR_TLS_INVALID_PROTOCOL_METHOD; using v8::Array; +using v8::ArrayBufferView; using v8::Boolean; using v8::ConstructorBehavior; using v8::Context; @@ -539,8 +540,9 @@ static BIOPointer LoadBIO(Environment* env, Local v) { return NodeBIO::NewFixed(*s, s.length()); } - if (Buffer::HasInstance(v)) { - return NodeBIO::NewFixed(Buffer::Data(v), Buffer::Length(v)); + if (v->IsArrayBufferView()) { + ArrayBufferViewContents buf(v.As()); + return NodeBIO::NewFixed(buf.data(), buf.length()); } return nullptr; @@ -1135,9 +1137,10 @@ void SecureContext::LoadPKCS12(const FunctionCallbackInfo& args) { if (args.Length() >= 2) { THROW_AND_RETURN_IF_NOT_BUFFER(env, args[1], "Pass phrase"); - size_t passlen = Buffer::Length(args[1]); + Local abv = args[1].As(); + size_t passlen = abv->ByteLength(); pass.resize(passlen + 1); - memcpy(pass.data(), Buffer::Data(args[1]), passlen); + abv->CopyContents(pass.data(), passlen); pass[passlen] = '\0'; } @@ -1261,15 +1264,16 @@ void SecureContext::SetTicketKeys(const FunctionCallbackInfo& args) { } THROW_AND_RETURN_IF_NOT_BUFFER(env, args[0], "Ticket keys"); + ArrayBufferViewContents buf(args[0].As()); - if (Buffer::Length(args[0]) != 48) { + if (buf.length() != 48) { return THROW_ERR_INVALID_ARG_VALUE( env, "Ticket keys length must be 48 bytes"); } - memcpy(wrap->ticket_key_name_, Buffer::Data(args[0]), 16); - memcpy(wrap->ticket_key_hmac_, Buffer::Data(args[0]) + 16, 16); - memcpy(wrap->ticket_key_aes_, Buffer::Data(args[0]) + 32, 16); + memcpy(wrap->ticket_key_name_, buf.data(), 16); + memcpy(wrap->ticket_key_hmac_, buf.data() + 16, 16); + memcpy(wrap->ticket_key_aes_, buf.data() + 32, 16); args.GetReturnValue().Set(true); #endif // !def(OPENSSL_NO_TLSEXT) && def(SSL_CTX_get_tlsext_ticket_keys) @@ -1349,29 +1353,29 @@ int SecureContext::TicketKeyCallback(SSL* ssl, return -1; } - memcpy(name, Buffer::Data(name_val), kTicketPartSize); - memcpy(iv, Buffer::Data(iv_val), kTicketPartSize); + name_val.As()->CopyContents(name, kTicketPartSize); + iv_val.As()->CopyContents(iv, kTicketPartSize); } + ArrayBufferViewContents hmac_buf(hmac); HMAC_Init_ex(hctx, - Buffer::Data(hmac), - Buffer::Length(hmac), + hmac_buf.data(), + hmac_buf.length(), EVP_sha256(), nullptr); - const unsigned char* aes_key = - reinterpret_cast(Buffer::Data(aes)); + ArrayBufferViewContents aes_key(aes.As()); if (enc) { EVP_EncryptInit_ex(ectx, EVP_aes_128_cbc(), nullptr, - aes_key, + aes_key.data(), iv); } else { EVP_DecryptInit_ex(ectx, EVP_aes_128_cbc(), nullptr, - aes_key, + aes_key.data(), iv); } @@ -2094,13 +2098,10 @@ void SSLWrap::SetSession(const FunctionCallbackInfo& args) { } THROW_AND_RETURN_IF_NOT_BUFFER(env, args[0], "Session"); - size_t slen = Buffer::Length(args[0]); - std::vector sbuf(slen); - if (char* p = Buffer::Data(args[0])) - sbuf.assign(p, p + slen); + ArrayBufferViewContents sbuf(args[0].As()); - const unsigned char* p = reinterpret_cast(sbuf.data()); - SSLSessionPointer sess(d2i_SSL_SESSION(nullptr, &p, slen)); + const unsigned char* p = sbuf.data(); + SSLSessionPointer sess(d2i_SSL_SESSION(nullptr, &p, sbuf.length())); if (sess == nullptr) return; @@ -2118,11 +2119,10 @@ void SSLWrap::LoadSession(const FunctionCallbackInfo& args) { ASSIGN_OR_RETURN_UNWRAP(&w, args.Holder()); if (args.Length() >= 1 && Buffer::HasInstance(args[0])) { - ssize_t slen = Buffer::Length(args[0]); - char* sbuf = Buffer::Data(args[0]); + ArrayBufferViewContents sbuf(args[0]); - const unsigned char* p = reinterpret_cast(sbuf); - SSL_SESSION* sess = d2i_SSL_SESSION(nullptr, &p, slen); + const unsigned char* p = sbuf.data(); + SSL_SESSION* sess = d2i_SSL_SESSION(nullptr, &p, sbuf.length()); // Setup next session and move hello to the BIO buffer w->next_sess_.reset(sess); @@ -2204,7 +2204,7 @@ void SSLWrap::SetOCSPResponse(const FunctionCallbackInfo& args) { THROW_AND_RETURN_IF_NOT_BUFFER(env, args[0], "OCSP response"); - w->ocsp_response_.Reset(args.GetIsolate(), args[0].As()); + w->ocsp_response_.Reset(args.GetIsolate(), args[0].As()); } @@ -2403,12 +2403,10 @@ int SSLWrap::SelectALPNCallback(SSL* s, w->object()->GetPrivate( env->context(), env->alpn_buffer_private_symbol()).ToLocalChecked(); - CHECK(Buffer::HasInstance(alpn_buffer)); - const unsigned char* alpn_protos = - reinterpret_cast(Buffer::Data(alpn_buffer)); - unsigned alpn_protos_len = Buffer::Length(alpn_buffer); + ArrayBufferViewContents alpn_protos(alpn_buffer); int status = SSL_select_next_proto(const_cast(out), outlen, - alpn_protos, alpn_protos_len, in, inlen); + alpn_protos.data(), alpn_protos.length(), + in, inlen); // According to 3.2. Protocol Selection of RFC7301, fatal // no_application_protocol alert shall be sent but OpenSSL 1.0.2 does not // support it yet. See @@ -2446,10 +2444,9 @@ void SSLWrap::SetALPNProtocols(const FunctionCallbackInfo& args) { return env->ThrowTypeError("Must give a Buffer as first argument"); if (w->is_client()) { - const unsigned char* alpn_protos = - reinterpret_cast(Buffer::Data(args[0])); - unsigned alpn_protos_len = Buffer::Length(args[0]); - int r = SSL_set_alpn_protos(w->ssl_.get(), alpn_protos, alpn_protos_len); + ArrayBufferViewContents alpn_protos(args[0]); + int r = SSL_set_alpn_protos( + w->ssl_.get(), alpn_protos.data(), alpn_protos.length()); CHECK_EQ(r, 0); } else { CHECK( @@ -2493,14 +2490,13 @@ int SSLWrap::TLSExtStatusCallback(SSL* s, void* arg) { if (w->ocsp_response_.IsEmpty()) return SSL_TLSEXT_ERR_NOACK; - Local obj = PersistentToLocal::Default(env->isolate(), - w->ocsp_response_); - char* resp = Buffer::Data(obj); - size_t len = Buffer::Length(obj); + Local obj = PersistentToLocal::Default(env->isolate(), + w->ocsp_response_); + size_t len = obj->ByteLength(); // OpenSSL takes control of the pointer after accepting it unsigned char* data = MallocOpenSSL(len); - memcpy(data, resp, len); + obj->CopyContents(data, len); if (!SSL_set_tlsext_status_ocsp_resp(s, data, len)) OPENSSL_free(data); @@ -2998,10 +2994,12 @@ ByteSource ByteSource::FromString(Environment* env, Local str, } ByteSource ByteSource::FromBuffer(Local buffer, bool ntc) { - size_t size = Buffer::Length(buffer); + CHECK(buffer->IsArrayBufferView()); + Local abv = buffer.As(); + size_t size = abv->ByteLength(); if (ntc) { char* data = MallocOpenSSL(size + 1); - memcpy(data, Buffer::Data(buffer), size); + abv->CopyContents(data, size); data[size] = 0; return Allocated(data, size); } @@ -3404,7 +3402,8 @@ void KeyObject::Init(const FunctionCallbackInfo& args) { switch (key->key_type_) { case kKeyTypeSecret: CHECK_EQ(args.Length(), 1); - key->InitSecret(Buffer::Data(args[0]), Buffer::Length(args[0])); + CHECK(args[0]->IsArrayBufferView()); + key->InitSecret(args[0].As()); break; case kKeyTypePublic: CHECK_EQ(args.Length(), 3); @@ -3429,11 +3428,12 @@ void KeyObject::Init(const FunctionCallbackInfo& args) { } } -void KeyObject::InitSecret(const char* key, size_t key_len) { +void KeyObject::InitSecret(v8::Local abv) { CHECK_EQ(this->key_type_, kKeyTypeSecret); + size_t key_len = abv->ByteLength(); char* mem = MallocOpenSSL(key_len); - memcpy(mem, key, key_len); + abv->CopyContents(mem, key_len); this->symmetric_key_ = std::unique_ptr>(mem, [key_len](char* p) { OPENSSL_clear_free(p, key_len); @@ -3643,8 +3643,7 @@ void CipherBase::Init(const FunctionCallbackInfo& args) { CHECK_GE(args.Length(), 3); const node::Utf8Value cipher_type(args.GetIsolate(), args[0]); - const char* key_buf = Buffer::Data(args[1]); - ssize_t key_buf_len = Buffer::Length(args[1]); + ArrayBufferViewContents key_buf(args[1]); // Don't assign to cipher->auth_tag_len_ directly; the value might not // represent a valid length at this point. @@ -3656,7 +3655,7 @@ void CipherBase::Init(const FunctionCallbackInfo& args) { auth_tag_len = kNoAuthTagLength; } - cipher->Init(*cipher_type, key_buf, key_buf_len, auth_tag_len); + cipher->Init(*cipher_type, key_buf.data(), key_buf.length(), auth_tag_len); } void CipherBase::InitIv(const char* cipher_type, @@ -3712,14 +3711,12 @@ void CipherBase::InitIv(const FunctionCallbackInfo& args) { const node::Utf8Value cipher_type(env->isolate(), args[0]); const ByteSource key = GetSecretKeyBytes(env, args[1]); - ssize_t iv_len; - const unsigned char* iv_buf; - if (args[2]->IsNull()) { - iv_buf = nullptr; - iv_len = -1; - } else { - iv_buf = reinterpret_cast(Buffer::Data(args[2])); - iv_len = Buffer::Length(args[2]); + ArrayBufferViewContents iv_buf; + ssize_t iv_len = -1; + if (!args[2]->IsNull()) { + CHECK(args[2]->IsArrayBufferView()); + iv_buf.Read(args[2].As()); + iv_len = iv_buf.length(); } // Don't assign to cipher->auth_tag_len_ directly; the value might not @@ -3735,7 +3732,7 @@ void CipherBase::InitIv(const FunctionCallbackInfo& args) { cipher->InitIv(*cipher_type, reinterpret_cast(key.get()), key.size(), - iv_buf, + iv_buf.data(), iv_len, auth_tag_len); } @@ -3889,7 +3886,8 @@ void CipherBase::SetAuthTag(const FunctionCallbackInfo& args) { CHECK_LE(cipher->auth_tag_len_, sizeof(cipher->auth_tag_)); memset(cipher->auth_tag_, 0, sizeof(cipher->auth_tag_)); - memcpy(cipher->auth_tag_, Buffer::Data(args[0]), cipher->auth_tag_len_); + args[0].As()->CopyContents( + cipher->auth_tag_, cipher->auth_tag_len_); args.GetReturnValue().Set(true); } @@ -3953,9 +3951,9 @@ void CipherBase::SetAAD(const FunctionCallbackInfo& args) { CHECK_EQ(args.Length(), 2); CHECK(args[1]->IsInt32()); int plaintext_len = args[1].As()->Value(); + ArrayBufferViewContents buf(args[0]); - bool b = cipher->SetAAD(Buffer::Data(args[0]), Buffer::Length(args[0]), - plaintext_len); + bool b = cipher->SetAAD(buf.data(), buf.length(), plaintext_len); args.GetReturnValue().Set(b); // Possibly report invalid state failure } @@ -4028,9 +4026,8 @@ void CipherBase::Update(const FunctionCallbackInfo& args) { return; r = cipher->Update(decoder.out(), decoder.size(), &out); } else { - char* buf = Buffer::Data(args[0]); - size_t buflen = Buffer::Length(args[0]); - r = cipher->Update(buf, buflen, &out); + ArrayBufferViewContents buf(args[0]); + r = cipher->Update(buf.data(), buf.length(), &out); } if (r != kSuccess) { @@ -4213,10 +4210,8 @@ void Hmac::HmacUpdate(const FunctionCallbackInfo& args) { r = hmac->HmacUpdate(decoder.out(), decoder.size()); } } else { - CHECK(args[0]->IsArrayBufferView()); - char* buf = Buffer::Data(args[0]); - size_t buflen = Buffer::Length(args[0]); - r = hmac->HmacUpdate(buf, buflen); + ArrayBufferViewContents buf(args[0]); + r = hmac->HmacUpdate(buf.data(), buf.length()); } args.GetReturnValue().Set(r); @@ -4324,9 +4319,8 @@ void Hash::HashUpdate(const FunctionCallbackInfo& args) { } r = hash->HashUpdate(decoder.out(), decoder.size()); } else if (args[0]->IsArrayBufferView()) { - char* buf = Buffer::Data(args[0]); - size_t buflen = Buffer::Length(args[0]); - r = hash->HashUpdate(buf, buflen); + ArrayBufferViewContents buf(args[0].As()); + r = hash->HashUpdate(buf.data(), buf.length()); } args.GetReturnValue().Set(r); @@ -4487,9 +4481,8 @@ void Sign::SignUpdate(const FunctionCallbackInfo& args) { ASSIGN_OR_RETURN_UNWRAP(&sign, args.Holder()); Error err; - char* buf = Buffer::Data(args[0]); - size_t buflen = Buffer::Length(args[0]); - err = sign->Update(buf, buflen); + ArrayBufferViewContents buf(args[0]); + err = sign->Update(buf.data(), buf.length()); sign->CheckThrow(err); } @@ -4634,9 +4627,8 @@ void Verify::VerifyUpdate(const FunctionCallbackInfo& args) { ASSIGN_OR_RETURN_UNWRAP(&verify, args.Holder()); Error err; - char* buf = Buffer::Data(args[0]); - size_t buflen = Buffer::Length(args[0]); - err = verify->Update(buf, buflen); + ArrayBufferViewContents buf(args[0]); + err = verify->Update(buf.data(), buf.length()); verify->CheckThrow(err); } @@ -4688,8 +4680,7 @@ void Verify::VerifyFinal(const FunctionCallbackInfo& args) { if (!pkey) return; - char* hbuf = Buffer::Data(args[offset]); - ssize_t hlen = Buffer::Length(args[offset]); + ArrayBufferViewContents hbuf(args[offset]); CHECK(args[offset + 1]->IsInt32()); int padding = args[offset + 1].As()->Value(); @@ -4698,8 +4689,8 @@ void Verify::VerifyFinal(const FunctionCallbackInfo& args) { int salt_len = args[offset + 2].As()->Value(); bool verify_result; - Error err = verify->VerifyFinal(pkey, hbuf, hlen, padding, salt_len, - &verify_result); + Error err = verify->VerifyFinal(pkey, hbuf.data(), hbuf.length(), padding, + salt_len, &verify_result); if (err != kSignOk) return verify->CheckThrow(err); args.GetReturnValue().Set(verify_result); @@ -4753,8 +4744,7 @@ void PublicKeyCipher::Cipher(const FunctionCallbackInfo& args) { return; THROW_AND_RETURN_IF_NOT_BUFFER(env, args[offset], "Data"); - char* buf = Buffer::Data(args[offset]); - ssize_t len = Buffer::Length(args[offset]); + ArrayBufferViewContents buf(args[offset]); uint32_t padding; if (!args[offset + 1]->Uint32Value(env->context()).To(&padding)) return; @@ -4767,8 +4757,8 @@ void PublicKeyCipher::Cipher(const FunctionCallbackInfo& args) { env, pkey, padding, - reinterpret_cast(buf), - len, + buf.data(), + buf.length(), &out); if (!r) @@ -4906,15 +4896,15 @@ void DiffieHellman::New(const FunctionCallbackInfo& args) { args[1].As()->Value()); } } else { + ArrayBufferViewContents arg0(args[0]); if (args[1]->IsInt32()) { - initialized = diffieHellman->Init(Buffer::Data(args[0]), - Buffer::Length(args[0]), + initialized = diffieHellman->Init(arg0.data(), + arg0.length(), args[1].As()->Value()); } else { - initialized = diffieHellman->Init(Buffer::Data(args[0]), - Buffer::Length(args[0]), - Buffer::Data(args[1]), - Buffer::Length(args[1])); + ArrayBufferViewContents arg1(args[1]); + initialized = diffieHellman->Init(arg0.data(), arg0.length(), + arg1.data(), arg1.length()); } } } @@ -5017,10 +5007,8 @@ void DiffieHellman::ComputeSecret(const FunctionCallbackInfo& args) { } THROW_AND_RETURN_IF_NOT_BUFFER(env, args[0], "Other party's public key"); - BignumPointer key(BN_bin2bn( - reinterpret_cast(Buffer::Data(args[0])), - Buffer::Length(args[0]), - nullptr)); + ArrayBufferViewContents key_buf(args[0].As()); + BignumPointer key(BN_bin2bn(key_buf.data(), key_buf.length(), nullptr)); AllocatedBuffer ret = env->AllocateManaged(DH_size(diffieHellman->dh_.get())); @@ -5087,9 +5075,9 @@ void DiffieHellman::SetKey(const FunctionCallbackInfo& args, return THROW_ERR_INVALID_ARG_TYPE(env, errmsg); } + ArrayBufferViewContents buf(args[0].As()); BIGNUM* num = - BN_bin2bn(reinterpret_cast(Buffer::Data(args[0])), - Buffer::Length(args[0]), nullptr); + BN_bin2bn(buf.data(), buf.length(), nullptr); CHECK_NOT_NULL(num); CHECK_EQ(1, set_field(dh->dh_.get(), num)); } @@ -5188,8 +5176,7 @@ void ECDH::GenerateKeys(const FunctionCallbackInfo& args) { ECPointPointer ECDH::BufferToPoint(Environment* env, const EC_GROUP* group, - char* data, - size_t len) { + Local buf) { int r; ECPointPointer pub(EC_POINT_new(group)); @@ -5198,11 +5185,12 @@ ECPointPointer ECDH::BufferToPoint(Environment* env, return pub; } + ArrayBufferViewContents input(buf); r = EC_POINT_oct2point( group, pub.get(), - reinterpret_cast(data), - len, + input.data(), + input.length(), nullptr); if (!r) return ECPointPointer(); @@ -5227,8 +5215,7 @@ void ECDH::ComputeSecret(const FunctionCallbackInfo& args) { ECPointPointer pub( ECDH::BufferToPoint(env, ecdh->group_, - Buffer::Data(args[0]), - Buffer::Length(args[0]))); + args[0])); if (!pub) { args.GetReturnValue().Set( FIXED_ONE_BYTE_STRING(env->isolate(), @@ -5360,8 +5347,7 @@ void ECDH::SetPublicKey(const FunctionCallbackInfo& args) { ECPointPointer pub( ECDH::BufferToPoint(env, ecdh->group_, - Buffer::Data(args[0].As()), - Buffer::Length(args[0].As()))); + args[0])); if (!pub) return env->ThrowError("Failed to convert Buffer to EC_POINT"); @@ -5427,8 +5413,10 @@ void CryptoJob::Run(std::unique_ptr job, Local wrap) { inline void CopyBuffer(Local buf, std::vector* vec) { + CHECK(buf->IsArrayBufferView()); vec->clear(); - if (auto p = Buffer::Data(buf)) vec->assign(p, p + Buffer::Length(buf)); + vec->resize(buf.As()->ByteLength()); + buf.As()->CopyContents(vec->data(), vec->size()); } @@ -6033,14 +6021,13 @@ bool VerifySpkac(const char* data, unsigned int len) { void VerifySpkac(const FunctionCallbackInfo& args) { bool verify_result = false; - size_t length = Buffer::Length(args[0]); - if (length == 0) - return args.GetReturnValue().Set(verify_result); + ArrayBufferViewContents input(args[0]); + if (input.length() == 0) + return args.GetReturnValue().SetEmptyString(); - char* data = Buffer::Data(args[0]); - CHECK_NOT_NULL(data); + CHECK_NOT_NULL(input.data()); - verify_result = VerifySpkac(data, length); + verify_result = VerifySpkac(input.data(), input.length()); args.GetReturnValue().Set(verify_result); } @@ -6075,15 +6062,15 @@ AllocatedBuffer ExportPublicKey(Environment* env, void ExportPublicKey(const FunctionCallbackInfo& args) { Environment* env = Environment::GetCurrent(args); - size_t length = Buffer::Length(args[0]); - if (length == 0) + ArrayBufferViewContents input(args[0]); + if (input.length() == 0) return args.GetReturnValue().SetEmptyString(); - char* data = Buffer::Data(args[0]); - CHECK_NOT_NULL(data); + CHECK_NOT_NULL(input.data()); size_t pkey_size; - AllocatedBuffer pkey = ExportPublicKey(env, data, length, &pkey_size); + AllocatedBuffer pkey = + ExportPublicKey(env, input.data(), input.length(), &pkey_size); if (pkey.data() == nullptr) return args.GetReturnValue().SetEmptyString(); @@ -6130,8 +6117,9 @@ void ConvertKey(const FunctionCallbackInfo& args) { Environment* env = Environment::GetCurrent(args); CHECK_EQ(args.Length(), 3); + CHECK(args[0]->IsArrayBufferView()); - size_t len = Buffer::Length(args[0]); + size_t len = args[0].As()->ByteLength(); if (len == 0) return args.GetReturnValue().SetEmptyString(); @@ -6149,8 +6137,7 @@ void ConvertKey(const FunctionCallbackInfo& args) { ECPointPointer pub( ECDH::BufferToPoint(env, group.get(), - Buffer::Data(args[0]), - len)); + args[0])); if (pub == nullptr) return env->ThrowError("Failed to convert Buffer to EC_POINT"); diff --git a/src/node_crypto.h b/src/node_crypto.h index 78293e70f1d794..bc9b05b616e173 100644 --- a/src/node_crypto.h +++ b/src/node_crypto.h @@ -335,7 +335,7 @@ class SSLWrap { ClientHelloParser hello_parser_; - Persistent ocsp_response_; + Persistent ocsp_response_; Persistent sni_context_; friend class SecureContext; @@ -464,7 +464,7 @@ class KeyObject : public BaseObject { static void New(const v8::FunctionCallbackInfo& args); static void Init(const v8::FunctionCallbackInfo& args); - void InitSecret(const char* key, size_t key_len); + void InitSecret(v8::Local abv); void InitPublic(const ManagedEVPPKey& pkey); void InitPrivate(const ManagedEVPPKey& pkey); @@ -805,8 +805,7 @@ class ECDH : public BaseObject { static void Initialize(Environment* env, v8::Local target); static ECPointPointer BufferToPoint(Environment* env, const EC_GROUP* group, - char* data, - size_t len); + v8::Local buf); // TODO(joyeecheung): track the memory used by OpenSSL types SET_NO_MEMORY_INFO() diff --git a/src/node_http2.cc b/src/node_http2.cc index 7fc21c9cae1eee..8946c71fc3e430 100644 --- a/src/node_http2.cc +++ b/src/node_http2.cc @@ -12,6 +12,7 @@ namespace node { using v8::ArrayBuffer; +using v8::ArrayBufferView; using v8::Boolean; using v8::Context; using v8::Float64Array; @@ -2483,7 +2484,7 @@ void Http2Session::Request(const FunctionCallbackInfo& args) { // state of the Http2Session, it's simply a notification. void Http2Session::Goaway(uint32_t code, int32_t lastStreamID, - uint8_t* data, + const uint8_t* data, size_t len) { if (IsDestroyed()) return; @@ -2508,16 +2509,13 @@ void Http2Session::Goaway(const FunctionCallbackInfo& args) { uint32_t code = args[0]->Uint32Value(context).ToChecked(); int32_t lastStreamID = args[1]->Int32Value(context).ToChecked(); - Local opaqueData = args[2]; - uint8_t* data = nullptr; - size_t length = 0; + ArrayBufferViewContents opaque_data; - if (Buffer::HasInstance(opaqueData)) { - data = reinterpret_cast(Buffer::Data(opaqueData)); - length = Buffer::Length(opaqueData); + if (args[2]->IsArrayBufferView()) { + opaque_data.Read(args[2].As()); } - session->Goaway(code, lastStreamID, data, length); + session->Goaway(code, lastStreamID, opaque_data.data(), opaque_data.length()); } // Update accounting of data chunks. This is used primarily to manage timeout @@ -2771,10 +2769,10 @@ void Http2Session::Ping(const FunctionCallbackInfo& args) { // A PING frame may have exactly 8 bytes of payload data. If not provided, // then the current hrtime will be used as the payload. - uint8_t* payload = nullptr; - if (Buffer::HasInstance(args[0])) { - payload = reinterpret_cast(Buffer::Data(args[0])); - CHECK_EQ(Buffer::Length(args[0]), 8); + ArrayBufferViewContents payload; + if (args[0]->IsArrayBufferView()) { + payload.Read(args[0].As()); + CHECK_EQ(payload.length(), 8); } Local obj; @@ -2799,7 +2797,7 @@ void Http2Session::Ping(const FunctionCallbackInfo& args) { // the callback will be invoked and a notification sent out to JS land. The // notification will include the duration of the ping, allowing the round // trip to be measured. - ping->Send(payload); + ping->Send(payload.data()); args.GetReturnValue().Set(true); } @@ -2871,7 +2869,7 @@ Http2Session::Http2Ping::Http2Ping(Http2Session* session, Local obj) session_(session), startTime_(uv_hrtime()) {} -void Http2Session::Http2Ping::Send(uint8_t* payload) { +void Http2Session::Http2Ping::Send(const uint8_t* payload) { uint8_t data[8]; if (payload == nullptr) { memcpy(&data, &startTime_, arraysize(data)); diff --git a/src/node_http2.h b/src/node_http2.h index aa953667facccf..83a73672942a71 100644 --- a/src/node_http2.h +++ b/src/node_http2.h @@ -699,7 +699,8 @@ class Http2Session : public AsyncWrap, public StreamListener { void Close(uint32_t code = NGHTTP2_NO_ERROR, bool socket_closed = false); void Consume(Local external); - void Goaway(uint32_t code, int32_t lastStreamID, uint8_t* data, size_t len); + void Goaway(uint32_t code, int32_t lastStreamID, + const uint8_t* data, size_t len); void AltSvc(int32_t id, uint8_t* origin, size_t origin_len, @@ -1089,7 +1090,7 @@ class Http2Session::Http2Ping : public AsyncWrap { SET_MEMORY_INFO_NAME(Http2Ping) SET_SELF_SIZE(Http2Ping) - void Send(uint8_t* payload); + void Send(const uint8_t* payload); void Done(bool ack, const uint8_t* payload = nullptr); private: diff --git a/src/string_decoder.cc b/src/string_decoder.cc index 9cf1bd671b3fc9..1441ca86936a5a 100644 --- a/src/string_decoder.cc +++ b/src/string_decoder.cc @@ -4,6 +4,7 @@ #include "string_decoder-inl.h" using v8::Array; +using v8::ArrayBufferView; using v8::Context; using v8::FunctionCallbackInfo; using v8::Integer; @@ -252,9 +253,13 @@ void DecodeData(const FunctionCallbackInfo& args) { StringDecoder* decoder = reinterpret_cast(Buffer::Data(args[0])); CHECK_NOT_NULL(decoder); - size_t nread = Buffer::Length(args[1]); + + CHECK(args[1]->IsArrayBufferView()); + ArrayBufferViewContents content(args[1].As()); + size_t length = content.length(); + MaybeLocal ret = - decoder->DecodeData(args.GetIsolate(), Buffer::Data(args[1]), &nread); + decoder->DecodeData(args.GetIsolate(), content.data(), &length); if (!ret.IsEmpty()) args.GetReturnValue().Set(ret.ToLocalChecked()); } diff --git a/src/util-inl.h b/src/util-inl.h index 9f57b635db3357..0b174170cac56c 100644 --- a/src/util-inl.h +++ b/src/util-inl.h @@ -466,6 +466,32 @@ SlicedArguments::SlicedArguments( (*this)[i] = args[i + start]; } +template +ArrayBufferViewContents::ArrayBufferViewContents( + v8::Local value) { + CHECK(value->IsArrayBufferView()); + Read(value.As()); +} + +template +ArrayBufferViewContents::ArrayBufferViewContents( + v8::Local abv) { + Read(abv); +} + +template +void ArrayBufferViewContents::Read(v8::Local abv) { + static_assert(sizeof(T) == 1, "Only supports one-byte data at the moment"); + length_ = abv->ByteLength(); + if (length_ > sizeof(stack_storage_) || abv->HasBuffer()) { + data_ = static_cast(abv->Buffer()->GetContents().Data()) + + abv->ByteOffset(); + } else { + abv->CopyContents(stack_storage_, sizeof(stack_storage_)); + data_ = stack_storage_; + } +} + } // namespace node #endif // defined(NODE_WANT_INTERNALS) && NODE_WANT_INTERNALS diff --git a/src/util.cc b/src/util.cc index 81070c239d8ad4..f6fd8a8b396ec5 100644 --- a/src/util.cc +++ b/src/util.cc @@ -29,6 +29,7 @@ namespace node { +using v8::ArrayBufferView; using v8::Isolate; using v8::Local; using v8::String; @@ -89,11 +90,11 @@ BufferValue::BufferValue(Isolate* isolate, Local value) { if (value->IsString()) { MakeUtf8String(isolate, value, this); - } else if (Buffer::HasInstance(value)) { - const size_t len = Buffer::Length(value); + } else if (value->IsArrayBufferView()) { + const size_t len = value.As()->ByteLength(); // Leave place for the terminating '\0' byte. AllocateSufficientStorage(len + 1); - memcpy(out(), Buffer::Data(value), len); + value.As()->CopyContents(out(), len); SetLengthAndZeroTerminate(len); } else { Invalidate(); diff --git a/src/util.h b/src/util.h index 312f91e68e8b97..ab54adbf2c4200 100644 --- a/src/util.h +++ b/src/util.h @@ -417,6 +417,27 @@ class MaybeStackBuffer { T buf_st_[kStackStorageSize]; }; +// Provides access to an ArrayBufferView's storage, either the original, +// or for small data, a copy of it. This object's lifetime is bound to the +// original ArrayBufferView's lifetime. +template +class ArrayBufferViewContents { + public: + ArrayBufferViewContents() {} + + explicit inline ArrayBufferViewContents(v8::Local value); + explicit inline ArrayBufferViewContents(v8::Local abv); + inline void Read(v8::Local abv); + + inline const T* data() const { return data_; } + inline size_t length() const { return length_; } + + private: + T stack_storage_[kStackStorageSize]; + T* data_ = nullptr; + size_t length_ = 0; +}; + class Utf8Value : public MaybeStackBuffer { public: explicit Utf8Value(v8::Isolate* isolate, v8::Local value); From 6b860593738d2a2a5b792c91ab1cd588e51f9b76 Mon Sep 17 00:00:00 2001 From: Anna Henningsen Date: Mon, 25 Feb 2019 04:41:53 +0100 Subject: [PATCH 3/4] buffer: avoid materializing ArrayBuffer for creation MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Do not create an `ArrayBuffer` if the engine’s settings avoid it and we don’t need it. --- lib/buffer.js | 8 ++------ 1 file changed, 2 insertions(+), 6 deletions(-) diff --git a/lib/buffer.js b/lib/buffer.js index 8b7aba1aaf06df..9684eaa6baeff0 100644 --- a/lib/buffer.js +++ b/lib/buffer.js @@ -105,13 +105,9 @@ let poolSize, poolOffset, allocPool; const zeroFill = bindingZeroFill || [0]; function createUnsafeBuffer(size) { - return new FastBuffer(createUnsafeArrayBuffer(size)); -} - -function createUnsafeArrayBuffer(size) { zeroFill[0] = 0; try { - return new ArrayBuffer(size); + return new FastBuffer(size); } finally { zeroFill[0] = 1; } @@ -119,7 +115,7 @@ function createUnsafeArrayBuffer(size) { function createPool() { poolSize = Buffer.poolSize; - allocPool = createUnsafeArrayBuffer(poolSize); + allocPool = createUnsafeBuffer(poolSize).buffer; poolOffset = 0; } createPool(); From 3f96c4d9de6df7e23f57ca436b6686141e60c88e Mon Sep 17 00:00:00 2001 From: Anna Henningsen Date: Mon, 25 Feb 2019 19:40:24 +0100 Subject: [PATCH 4/4] test: verify heap buffer allocations occur Check that small typed arrays, including `Buffer`s (unless allocated by `Buffer.allocUnsafe()`), are indeed heap-allocated. --- src/node_util.cc | 7 ++++ .../test-buffer-backing-arraybuffer.js | 37 +++++++++++++++++++ 2 files changed, 44 insertions(+) create mode 100644 test/parallel/test-buffer-backing-arraybuffer.js diff --git a/src/node_util.cc b/src/node_util.cc index f2c008c797d61b..6d20f636f0d25e 100644 --- a/src/node_util.cc +++ b/src/node_util.cc @@ -8,6 +8,7 @@ namespace util { using v8::ALL_PROPERTIES; using v8::Array; +using v8::ArrayBufferView; using v8::Boolean; using v8::Context; using v8::Function; @@ -174,6 +175,11 @@ void WatchdogHasPendingSigint(const FunctionCallbackInfo& args) { args.GetReturnValue().Set(ret); } +void ArrayBufferViewHasBuffer(const FunctionCallbackInfo& args) { + CHECK(args[0]->IsArrayBufferView()); + args.GetReturnValue().Set(args[0].As()->HasBuffer()); +} + void EnqueueMicrotask(const FunctionCallbackInfo& args) { Environment* env = Environment::GetCurrent(args); Isolate* isolate = env->isolate(); @@ -254,6 +260,7 @@ void Initialize(Local target, env->SetMethodNoSideEffect(target, "watchdogHasPendingSigint", WatchdogHasPendingSigint); + env->SetMethod(target, "arrayBufferViewHasBuffer", ArrayBufferViewHasBuffer); env->SetMethod(target, "enqueueMicrotask", EnqueueMicrotask); env->SetMethod(target, "triggerFatalException", FatalException); Local constants = Object::New(env->isolate()); diff --git a/test/parallel/test-buffer-backing-arraybuffer.js b/test/parallel/test-buffer-backing-arraybuffer.js new file mode 100644 index 00000000000000..e7e15c079e6332 --- /dev/null +++ b/test/parallel/test-buffer-backing-arraybuffer.js @@ -0,0 +1,37 @@ +// Flags: --expose-internals +'use strict'; +require('../common'); +const assert = require('assert'); +const { internalBinding } = require('internal/test/binding'); +const { arrayBufferViewHasBuffer } = internalBinding('util'); + +const tests = [ + { length: 0, expectOnHeap: true }, + { length: 48, expectOnHeap: true }, + { length: 96, expectOnHeap: false }, + { length: 1024, expectOnHeap: false }, +]; + +for (const { length, expectOnHeap } of tests) { + const arrays = [ + new Uint8Array(length), + new Uint16Array(length / 2), + new Uint32Array(length / 4), + new Float32Array(length / 4), + new Float64Array(length / 8), + Buffer.alloc(length), + Buffer.allocUnsafeSlow(length) + // Buffer.allocUnsafe() is missing because it may use pooled allocations. + ]; + + for (const array of arrays) { + const isOnHeap = !arrayBufferViewHasBuffer(array); + assert.strictEqual(isOnHeap, expectOnHeap, + `mismatch: ${isOnHeap} vs ${expectOnHeap} ` + + `for ${array.constructor.name}, length = ${length}`); + + // Consistency check: Accessing .buffer should create it. + array.buffer; + assert(arrayBufferViewHasBuffer(array)); + } +}