src: optimize ALPN callback

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: #44875 Reviewed-By: James M Snell <[email protected]> Reviewed-By: Tobias Nießen <[email protected]>
nodejs · Nov 10, 2022 · e5b1179 · e5b1179
1 parent 9dc21a1
commit e5b1179
Show file tree

Hide file tree

Showing 5 changed files with 19 additions and 35 deletions.
diff --git 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
@@ -225,26 +225,17 @@ int SelectALPNCallback(
     const unsigned char* in,
     unsigned int inlen,
     void* arg) {
-  TLSWrap* w = static_cast<TLSWrap*>(SSL_get_app_data(s));
-  Environment* env = w->env();
-  HandleScope handle_scope(env->isolate());
-  Context::Scope context_scope(env->context());
+  TLSWrap* w = static_cast<TLSWrap*>(arg);
+  const std::vector<unsigned char>& alpn_protos = w->alpn_protos_;
 
-  Local<Value> alpn_buffer =
-      w->object()->GetPrivate(
-          env->context(),
-          env->alpn_buffer_private_symbol()).FromMaybe(Local<Value>());
-  if (UNLIKELY(alpn_buffer.IsEmpty()) || !alpn_buffer->IsArrayBufferView())
-    return SSL_TLSEXT_ERR_NOACK;
+  if (alpn_protos.empty()) return SSL_TLSEXT_ERR_NOACK;
 
-  ArrayBufferViewContents<unsigned char> alpn_protos(alpn_buffer);
-  int status = SSL_select_next_proto(
-      const_cast<unsigned char**>(out),
-      outlen,
-      alpn_protos.data(),
-      alpn_protos.length(),
-      in,
-      inlen);
+  int status = SSL_select_next_proto(const_cast<unsigned char**>(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<Value>& args) {
   if (args.Length() < 1 || !Buffer::HasInstance(args[0]))
     return env->ThrowTypeError("Must give a Buffer as first argument");
 
+  ArrayBufferViewContents<uint8_t> protos(args[0].As<ArrayBufferView>());
   SSL* ssl = w->ssl_.get();
   if (w->is_client()) {
-    ArrayBufferViewContents<uint8_t> protos(args[0].As<ArrayBufferView>());
     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<unsigned char>(
+        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
@@ -34,6 +34,7 @@
 #include <openssl/ssl.h>
 
 #include <string>
+#include <vector>
 
 namespace node {
 namespace crypto {
@@ -283,6 +284,9 @@ class TLSWrap : public AsyncWrap,
   void* cert_cb_arg_ = nullptr;
 
   BIOPointer bio_trace_;
+
+ public:
+  std::vector<unsigned char> alpn_protos_;  // Accessed by SelectALPNCallback.
 };
 
 }  // namespace crypto

diff --git 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
@@ -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;