Skip to content

Commit 0f58967

Browse files
bnoordhuisbnoordhuis
committed
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.
1 parent 432ca36 commit 0f58967

5 files changed

Lines changed: 19 additions & 35 deletions

File tree

‎lib/_tls_wrap.js‎

Lines changed: 2 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -786,11 +786,8 @@ TLSSocket.prototype._init = function(socket, wrap) {
786786
ssl.enableCertCb();
787787
}
788788

789-
if (options.ALPNProtocols) {
790-
// Keep reference in secureContext not to be GC-ed
791-
ssl._secureContext.alpnBuffer = options.ALPNProtocols;
792-
ssl.setALPNProtocols(ssl._secureContext.alpnBuffer);
793-
}
789+
if (options.ALPNProtocols)
790+
ssl.setALPNProtocols(options.ALPNProtocols);
794791

795792
if (options.pskCallback && ssl.enablePskCallback) {
796793
validateFunction(options.pskCallback, 'pskCallback');

‎src/crypto/crypto_tls.cc‎

Lines changed: 13 additions & 28 deletions
Original file line numberDiff line numberDiff line change
@@ -225,26 +225,17 @@ int SelectALPNCallback(
225225
const unsigned char* in,
226226
unsigned int inlen,
227227
void* arg) {
228-
TLSWrap* w = static_cast<TLSWrap*>(SSL_get_app_data(s));
229-
Environment* env = w->env();
230-
HandleScope handle_scope(env->isolate());
231-
Context::Scope context_scope(env->context());
228+
TLSWrap* w = static_cast<TLSWrap*>(arg);
229+
const std::vector<unsigned char>& alpn_protos = w->alpn_protos_;
232230

233-
Local<Value> alpn_buffer =
234-
w->object()->GetPrivate(
235-
env->context(),
236-
env->alpn_buffer_private_symbol()).FromMaybe(Local<Value>());
237-
if (UNLIKELY(alpn_buffer.IsEmpty()) || !alpn_buffer->IsArrayBufferView())
238-
return SSL_TLSEXT_ERR_NOACK;
231+
if (alpn_protos.empty()) return SSL_TLSEXT_ERR_NOACK;
239232

240-
ArrayBufferViewContents<unsigned char> alpn_protos(alpn_buffer);
241-
int status = SSL_select_next_proto(
242-
const_cast<unsigned char**>(out),
243-
outlen,
244-
alpn_protos.data(),
245-
alpn_protos.length(),
246-
in,
247-
inlen);
233+
int status = SSL_select_next_proto(const_cast<unsigned char**>(out),
234+
outlen,
235+
alpn_protos.data(),
236+
alpn_protos.size(),
237+
in,
238+
inlen);
248239

249240
// Previous versions of Node.js returned SSL_TLSEXT_ERR_NOACK if no protocol
250241
// 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) {
15291520
if (args.Length() < 1 || !Buffer::HasInstance(args[0]))
15301521
return env->ThrowTypeError("Must give a Buffer as first argument");
15311522

1523+
ArrayBufferViewContents<uint8_t> protos(args[0].As<ArrayBufferView>());
15321524
SSL* ssl = w->ssl_.get();
15331525
if (w->is_client()) {
1534-
ArrayBufferViewContents<uint8_t> protos(args[0].As<ArrayBufferView>());
15351526
CHECK_EQ(0, SSL_set_alpn_protos(ssl, protos.data(), protos.length()));
15361527
} else {
1537-
CHECK(
1538-
w->object()->SetPrivate(
1539-
env->context(),
1540-
env->alpn_buffer_private_symbol(),
1541-
args[0]).FromJust());
1542-
// Server should select ALPN protocol from list of advertised by client
1543-
SSL_CTX_set_alpn_select_cb(SSL_get_SSL_CTX(ssl),
1544-
SelectALPNCallback,
1545-
nullptr);
1528+
w->alpn_protos_ = std::vector<unsigned char>(
1529+
protos.data(), protos.data() + protos.length());
1530+
SSL_CTX_set_alpn_select_cb(SSL_get_SSL_CTX(ssl), SelectALPNCallback, w);
15461531
}
15471532
}
15481533

‎src/crypto/crypto_tls.h‎

Lines changed: 4 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -34,6 +34,7 @@
3434
#include <openssl/ssl.h>
3535

3636
#include <string>
37+
#include <vector>
3738

3839
namespace node {
3940
namespace crypto {
@@ -283,6 +284,9 @@ class TLSWrap : public AsyncWrap,
283284
void* cert_cb_arg_ = nullptr;
284285

285286
BIOPointer bio_trace_;
287+
288+
public:
289+
std::vector<unsigned char> alpn_protos_; // Accessed by SelectALPNCallback.
286290
};
287291

288292
} // namespace crypto

‎src/env_properties.h‎

Lines changed: 0 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -18,7 +18,6 @@
1818
// for the sake of convenience. Strings should be ASCII-only and have a
1919
// "node:" prefix to avoid name clashes with third-party code.
2020
#define PER_ISOLATE_PRIVATE_SYMBOL_PROPERTIES(V) \
21-
V(alpn_buffer_private_symbol, "node:alpnBuffer") \
2221
V(arrow_message_private_symbol, "node:arrowMessage") \
2322
V(contextify_context_private_symbol, "node:contextify:context") \
2423
V(contextify_global_private_symbol, "node:contextify:global") \

‎typings/internalBinding/util.d.ts‎

Lines changed: 0 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -9,7 +9,6 @@ declare namespace InternalUtilBinding {
99

1010
declare function InternalBinding(binding: 'util'): {
1111
// PER_ISOLATE_PRIVATE_SYMBOL_PROPERTIES, defined in src/env.h
12-
alpn_buffer_private_symbol: 0;
1312
arrow_message_private_symbol: 1;
1413
contextify_context_private_symbol: 2;
1514
contextify_global_private_symbol: 3;

0 commit comments

Comments
 (0)