From e5b1179630930cbc32b8c470ef85b9ec07b0763e Mon Sep 17 00:00:00 2001 From: Ben Noordhuis Date: Mon, 3 Oct 2022 13:00:53 +0200 Subject: [PATCH] src: optimize ALPN callback MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit It doesn't make sense from a performance perspective to retain an arraybuffer with the ALPN byte string and look it up as a property on the JS context object for every TLS handshake. Store the byte string in the C++ TLSWrap object instead. That's both a lot faster and a lot simpler. PR-URL: https://github.com/nodejs/node/pull/44875 Reviewed-By: James M Snell Reviewed-By: Tobias Nießen --- lib/_tls_wrap.js | 7 ++---- src/crypto/crypto_tls.cc | 41 ++++++++++--------------------- src/crypto/crypto_tls.h | 4 +++ src/env_properties.h | 1 - typings/internalBinding/util.d.ts | 1 - 5 files changed, 19 insertions(+), 35 deletions(-) diff --git a/lib/_tls_wrap.js b/lib/_tls_wrap.js index 11ff14d725b36b..09e599404078d4 100644 --- a/lib/_tls_wrap.js +++ b/lib/_tls_wrap.js @@ -786,11 +786,8 @@ TLSSocket.prototype._init = function(socket, wrap) { ssl.enableCertCb(); } - if (options.ALPNProtocols) { - // Keep reference in secureContext not to be GC-ed - ssl._secureContext.alpnBuffer = options.ALPNProtocols; - ssl.setALPNProtocols(ssl._secureContext.alpnBuffer); - } + if (options.ALPNProtocols) + ssl.setALPNProtocols(options.ALPNProtocols); if (options.pskCallback && ssl.enablePskCallback) { validateFunction(options.pskCallback, 'pskCallback'); diff --git a/src/crypto/crypto_tls.cc b/src/crypto/crypto_tls.cc index f835c4f0add5e6..2fd32a8dcebbb8 100644 --- a/src/crypto/crypto_tls.cc +++ b/src/crypto/crypto_tls.cc @@ -225,26 +225,17 @@ int SelectALPNCallback( const unsigned char* in, unsigned int inlen, void* arg) { - TLSWrap* w = static_cast(SSL_get_app_data(s)); - Environment* env = w->env(); - HandleScope handle_scope(env->isolate()); - Context::Scope context_scope(env->context()); + TLSWrap* w = static_cast(arg); + const std::vector& alpn_protos = w->alpn_protos_; - Local alpn_buffer = - w->object()->GetPrivate( - env->context(), - env->alpn_buffer_private_symbol()).FromMaybe(Local()); - if (UNLIKELY(alpn_buffer.IsEmpty()) || !alpn_buffer->IsArrayBufferView()) - return SSL_TLSEXT_ERR_NOACK; + if (alpn_protos.empty()) return SSL_TLSEXT_ERR_NOACK; - ArrayBufferViewContents alpn_protos(alpn_buffer); - int status = SSL_select_next_proto( - const_cast(out), - outlen, - alpn_protos.data(), - alpn_protos.length(), - in, - inlen); + int status = SSL_select_next_proto(const_cast(out), + outlen, + alpn_protos.data(), + alpn_protos.size(), + in, + inlen); // Previous versions of Node.js returned SSL_TLSEXT_ERR_NOACK if no protocol // match was found. This would neither cause a fatal alert nor would it result @@ -1529,20 +1520,14 @@ void TLSWrap::SetALPNProtocols(const FunctionCallbackInfo& args) { if (args.Length() < 1 || !Buffer::HasInstance(args[0])) return env->ThrowTypeError("Must give a Buffer as first argument"); + ArrayBufferViewContents protos(args[0].As()); SSL* ssl = w->ssl_.get(); if (w->is_client()) { - ArrayBufferViewContents protos(args[0].As()); CHECK_EQ(0, SSL_set_alpn_protos(ssl, protos.data(), protos.length())); } else { - CHECK( - w->object()->SetPrivate( - env->context(), - env->alpn_buffer_private_symbol(), - args[0]).FromJust()); - // Server should select ALPN protocol from list of advertised by client - SSL_CTX_set_alpn_select_cb(SSL_get_SSL_CTX(ssl), - SelectALPNCallback, - nullptr); + w->alpn_protos_ = std::vector( + protos.data(), protos.data() + protos.length()); + SSL_CTX_set_alpn_select_cb(SSL_get_SSL_CTX(ssl), SelectALPNCallback, w); } } diff --git a/src/crypto/crypto_tls.h b/src/crypto/crypto_tls.h index 765c6c476b12e7..6a7e2f62a20004 100644 --- a/src/crypto/crypto_tls.h +++ b/src/crypto/crypto_tls.h @@ -34,6 +34,7 @@ #include #include +#include namespace node { namespace crypto { @@ -283,6 +284,9 @@ class TLSWrap : public AsyncWrap, void* cert_cb_arg_ = nullptr; BIOPointer bio_trace_; + + public: + std::vector alpn_protos_; // Accessed by SelectALPNCallback. }; } // namespace crypto diff --git a/src/env_properties.h b/src/env_properties.h index 76c52f1ea57385..c708518efa293f 100644 --- a/src/env_properties.h +++ b/src/env_properties.h @@ -18,7 +18,6 @@ // for the sake of convenience. Strings should be ASCII-only and have a // "node:" prefix to avoid name clashes with third-party code. #define PER_ISOLATE_PRIVATE_SYMBOL_PROPERTIES(V) \ - V(alpn_buffer_private_symbol, "node:alpnBuffer") \ V(arrow_message_private_symbol, "node:arrowMessage") \ V(contextify_context_private_symbol, "node:contextify:context") \ V(contextify_global_private_symbol, "node:contextify:global") \ diff --git a/typings/internalBinding/util.d.ts b/typings/internalBinding/util.d.ts index bb85acee21a458..5045330dc1547c 100644 --- a/typings/internalBinding/util.d.ts +++ b/typings/internalBinding/util.d.ts @@ -9,7 +9,6 @@ declare namespace InternalUtilBinding { declare function InternalBinding(binding: 'util'): { // PER_ISOLATE_PRIVATE_SYMBOL_PROPERTIES, defined in src/env.h - alpn_buffer_private_symbol: 0; arrow_message_private_symbol: 1; contextify_context_private_symbol: 2; contextify_global_private_symbol: 3;