Skip to content

Commit 73bce22

Browse files
RaisinTenRaisinTen
committed
src,crypto: refactor crypto_tls.*
By the design of `GetSSLError()`, the V8 API was unnecessarily being accessed in places where it eventually didn't get used. So this refactor inlines the function appropriately in places where it was being used. Also, this replaces uses of `AllocatedBuffers` with `BackingStore`s. Signed-off-by: Darshan Sen <darshan.sen@postman.com>
1 parent 47fb867 commit 73bce22

File tree

2 files changed

+101
-126
lines changed

2 files changed

+101
-126
lines changed

‎src/crypto/crypto_tls.cc‎

Lines changed: 99 additions & 123 deletions
Original file line numberDiff line numberDiff line change
@@ -37,10 +37,11 @@
3737
namespace node {
3838

3939
using v8::Array;
40+
using v8::ArrayBuffer;
4041
using v8::ArrayBufferView;
42+
using v8::BackingStore;
4143
using v8::Context;
4244
using v8::DontDelete;
43-
using v8::EscapableHandleScope;
4445
using v8::Exception;
4546
using v8::False;
4647
using v8::Function;
@@ -313,6 +314,16 @@ inline bool Set(
313314
OneByteString(env->isolate(), value))
314315
.IsNothing();
315316
}
317+
318+
std::string GetBIOError() {
319+
BIO* bio = BIO_new(BIO_s_mem());
320+
CHECK_NOT_NULL(bio);
321+
auto cleanup = OnScopeLeave([&]() { BIO_free_all(bio); });
322+
ERR_print_errors(bio);
323+
BUF_MEM* mem;
324+
BIO_get_mem_ptr(bio, &mem);
325+
return std::string(mem->data, mem->length);
326+
}
316327
} // namespace
317328

318329
TLSWrap::TLSWrap(Environment* env,
@@ -572,7 +583,8 @@ void TLSWrap::EncOut() {
572583
// No encrypted output ready to write to the underlying stream.
573584
if (BIO_pending(enc_out_) == 0) {
574585
Debug(this, "No pending encrypted output");
575-
if (pending_cleartext_input_.size() == 0) {
586+
if (!pending_cleartext_input_ ||
587+
pending_cleartext_input_->ByteLength() == 0) {
576588
if (!in_dowrite_) {
577589
Debug(this, "No pending cleartext input, not inside DoWrite()");
578590
InvokeQueued(0);
@@ -665,84 +677,9 @@ void TLSWrap::OnStreamAfterWrite(WriteWrap* req_wrap, int status) {
665677
EncOut();
666678
}
667679

668-
MaybeLocal<Value> TLSWrap::GetSSLError(int status, int* err, std::string* msg) {
669-
EscapableHandleScope scope(env()->isolate());
670-
671-
// ssl_ is already destroyed in reading EOF by close notify alert.
672-
if (ssl_ == nullptr)
673-
return MaybeLocal<Value>();
674-
675-
*err = SSL_get_error(ssl_.get(), status);
676-
switch (*err) {
677-
case SSL_ERROR_NONE:
678-
case SSL_ERROR_WANT_READ:
679-
case SSL_ERROR_WANT_WRITE:
680-
case SSL_ERROR_WANT_X509_LOOKUP:
681-
return MaybeLocal<Value>();
682-
683-
case SSL_ERROR_ZERO_RETURN:
684-
return scope.Escape(env()->zero_return_string());
685-
686-
case SSL_ERROR_SSL:
687-
case SSL_ERROR_SYSCALL:
688-
{
689-
unsigned long ssl_err = ERR_peek_error(); // NOLINT(runtime/int)
690-
BIO* bio = BIO_new(BIO_s_mem());
691-
ERR_print_errors(bio);
692-
693-
BUF_MEM* mem;
694-
BIO_get_mem_ptr(bio, &mem);
695-
696-
Isolate* isolate = env()->isolate();
697-
Local<Context> context = isolate->GetCurrentContext();
698-
699-
Local<String> message = OneByteString(isolate, mem->data, mem->length);
700-
Local<Value> exception = Exception::Error(message);
701-
Local<Object> obj =
702-
exception->ToObject(context).FromMaybe(Local<Object>());
703-
if (UNLIKELY(obj.IsEmpty()))
704-
return MaybeLocal<Value>();
705-
706-
const char* ls = ERR_lib_error_string(ssl_err);
707-
const char* fs = ERR_func_error_string(ssl_err);
708-
const char* rs = ERR_reason_error_string(ssl_err);
709-
710-
if (!Set(env(), obj, env()->library_string(), ls) ||
711-
!Set(env(), obj, env()->function_string(), fs)) {
712-
return MaybeLocal<Value>();
713-
}
714-
715-
if (rs != nullptr) {
716-
if (!Set(env(), obj, env()->reason_string(), rs))
717-
return MaybeLocal<Value>();
718-
719-
// SSL has no API to recover the error name from the number, so we
720-
// transform reason strings like "this error" to "ERR_SSL_THIS_ERROR",
721-
// which ends up being close to the original error macro name.
722-
std::string code(rs);
723-
724-
for (auto& c : code)
725-
c = (c == ' ') ? '_' : ToUpper(c);
726-
727-
if (!Set(env(), obj,
728-
env()->code_string(),
729-
("ERR_SSL_" + code).c_str())) {
730-
return MaybeLocal<Value>();
731-
}
732-
}
733-
734-
if (msg != nullptr)
735-
msg->assign(mem->data, mem->data + mem->length);
736-
737-
BIO_free_all(bio);
738-
739-
return scope.Escape(exception);
740-
}
741-
742-
default:
743-
UNREACHABLE();
744-
}
745-
UNREACHABLE();
680+
int TLSWrap::GetSSLError(int status) const {
681+
// ssl_ might already be destroyed for reading EOF from a close notify alert.
682+
return ssl_ != nullptr ? SSL_get_error(ssl_.get(), status) : 0;
746683
}
747684

748685
void TLSWrap::ClearOut() {
@@ -809,24 +746,61 @@ void TLSWrap::ClearOut() {
809746
// See node#1642 and SSL_read(3SSL) for details.
810747
if (read <= 0) {
811748
HandleScope handle_scope(env()->isolate());
812-
int err;
749+
Local<Value> exception;
750+
int err = GetSSLError(read);
751+
switch (err) {
752+
case SSL_ERROR_ZERO_RETURN:
753+
// Ignore ZERO_RETURN after EOF, it is basically not an error.
754+
if (eof_) return;
755+
exception = env()->zero_return_string();
756+
break;
813757

814-
Local<Value> arg = GetSSLError(read, &err, nullptr)
815-
.FromMaybe(Local<Value>());
758+
case SSL_ERROR_SSL:
759+
case SSL_ERROR_SYSCALL:
760+
{
761+
unsigned long ssl_err = ERR_peek_error(); // NOLINT(runtime/int)
762+
763+
Local<Context> context = env()->isolate()->GetCurrentContext();
764+
if (UNLIKELY(context.IsEmpty())) return;
765+
const std::string error_str = GetBIOError();
766+
Local<String> message = OneByteString(env()->isolate(),
767+
error_str.c_str(),
768+
error_str.size());
769+
if (UNLIKELY(message.IsEmpty())) return;
770+
exception = Exception::Error(message);
771+
if (UNLIKELY(exception.IsEmpty())) return;
772+
Local<Object> obj;
773+
if (UNLIKELY(!exception->ToObject(context).ToLocal(&obj))) return;
774+
775+
const char* ls = ERR_lib_error_string(ssl_err);
776+
const char* fs = ERR_func_error_string(ssl_err);
777+
const char* rs = ERR_reason_error_string(ssl_err);
778+
if (!Set(env(), obj, env()->library_string(), ls) ||
779+
!Set(env(), obj, env()->function_string(), fs) ||
780+
!Set(env(), obj, env()->reason_string(), rs, false)) return;
781+
// SSL has no API to recover the error name from the number, so we
782+
// transform reason strings like "this error" to "ERR_SSL_THIS_ERROR",
783+
// which ends up being close to the original error macro name.
784+
std::string code(rs);
785+
// TODO(RaisinTen): Pass an appropriate execution policy when it is
786+
// implemented in our supported compilers.
787+
std::transform(code.begin(), code.end(), code.begin(),
788+
[](char c) { return c == ' ' ? '_' : ToUpper(c); });
789+
if (!Set(env(), obj,
790+
env()->code_string(), ("ERR_SSL_" + code).c_str())) return;
791+
}
792+
break;
816793

817-
// Ignore ZERO_RETURN after EOF, it is basically not a error
818-
if (err == SSL_ERROR_ZERO_RETURN && eof_)
819-
return;
794+
default: return;
795+
}
820796

821-
if (LIKELY(!arg.IsEmpty())) {
822-
Debug(this, "Got SSL error (%d), calling onerror", err);
823-
// When TLS Alert are stored in wbio,
824-
// it should be flushed to socket before destroyed.
825-
if (BIO_pending(enc_out_) != 0)
826-
EncOut();
797+
Debug(this, "Got SSL error (%d), calling onerror", err);
798+
// When TLS Alert are stored in wbio,
799+
// it should be flushed to socket before destroyed.
800+
if (BIO_pending(enc_out_) != 0)
801+
EncOut();
827802

828-
MakeCallback(env()->onerror_string(), 1, &arg);
829-
}
803+
MakeCallback(env()->onerror_string(), 1, &exception);
830804
}
831805
}
832806

@@ -843,18 +817,19 @@ void TLSWrap::ClearIn() {
843817
return;
844818
}
845819

846-
if (pending_cleartext_input_.size() == 0) {
820+
if (!pending_cleartext_input_ ||
821+
pending_cleartext_input_->ByteLength() == 0) {
847822
Debug(this, "Returning from ClearIn(), no pending data");
848823
return;
849824
}
850825

851-
AllocatedBuffer data = std::move(pending_cleartext_input_);
826+
std::unique_ptr<BackingStore> bs = std::move(pending_cleartext_input_);
852827
MarkPopErrorOnReturn mark_pop_error_on_return;
853828

854-
NodeBIO::FromBIO(enc_out_)->set_allocate_tls_hint(data.size());
855-
int written = SSL_write(ssl_.get(), data.data(), data.size());
856-
Debug(this, "Writing %zu bytes, written = %d", data.size(), written);
857-
CHECK(written == -1 || written == static_cast<int>(data.size()));
829+
NodeBIO::FromBIO(enc_out_)->set_allocate_tls_hint(bs->ByteLength());
830+
int written = SSL_write(ssl_.get(), bs->Data(), bs->ByteLength());
831+
Debug(this, "Writing %zu bytes, written = %d", bs->ByteLength(), written);
832+
CHECK(written == -1 || written == static_cast<int>(bs->ByteLength()));
858833

859834
// All written
860835
if (written != -1) {
@@ -863,24 +838,20 @@ void TLSWrap::ClearIn() {
863838
}
864839

865840
// Error or partial write
866-
HandleScope handle_scope(env()->isolate());
867-
Context::Scope context_scope(env()->context());
868-
869-
int err;
870-
std::string error_str;
871-
MaybeLocal<Value> arg = GetSSLError(written, &err, &error_str);
872-
if (!arg.IsEmpty()) {
841+
int err = GetSSLError(written);
842+
if (err == SSL_ERROR_SSL || err == SSL_ERROR_SYSCALL) {
873843
Debug(this, "Got SSL error (%d)", err);
874844
write_callback_scheduled_ = true;
875845
// TODO(@sam-github) Should forward an error object with
876846
// .code/.function/.etc, if possible.
877-
return InvokeQueued(UV_EPROTO, error_str.c_str());
847+
InvokeQueued(UV_EPROTO, GetBIOError().c_str());
848+
return;
878849
}
879850

880851
Debug(this, "Pushing data back");
881852
// Push back the not-yet-written data. This can be skipped in the error
882853
// case because no further writes would succeed anyway.
883-
pending_cleartext_input_ = std::move(data);
854+
pending_cleartext_input_ = std::move(bs);
884855
}
885856

886857
std::string TLSWrap::diagnostic_name() const {
@@ -998,7 +969,7 @@ int TLSWrap::DoWrite(WriteWrap* w,
998969
return 0;
999970
}
1000971

1001-
AllocatedBuffer data;
972+
std::unique_ptr<BackingStore> bs;
1002973
MarkPopErrorOnReturn mark_pop_error_on_return;
1003974

1004975
int written = 0;
@@ -1012,36 +983,39 @@ int TLSWrap::DoWrite(WriteWrap* w,
1012983
// and copying it when it could just be used.
1013984

1014985
if (nonempty_count != 1) {
1015-
data = AllocatedBuffer::AllocateManaged(env(), length);
986+
{
987+
NoArrayBufferZeroFillScope no_zero_fill_scope(env()->isolate_data());
988+
bs = ArrayBuffer::NewBackingStore(env()->isolate(), length);
989+
}
1016990
size_t offset = 0;
1017991
for (i = 0; i < count; i++) {
1018-
memcpy(data.data() + offset, bufs[i].base, bufs[i].len);
992+
memcpy(static_cast<char*>(bs->Data()) + offset,
993+
bufs[i].base, bufs[i].len);
1019994
offset += bufs[i].len;
1020995
}
1021996

1022997
NodeBIO::FromBIO(enc_out_)->set_allocate_tls_hint(length);
1023-
written = SSL_write(ssl_.get(), data.data(), length);
998+
written = SSL_write(ssl_.get(), bs->Data(), length);
1024999
} else {
10251000
// Only one buffer: try to write directly, only store if it fails
10261001
uv_buf_t* buf = &bufs[nonempty_i];
10271002
NodeBIO::FromBIO(enc_out_)->set_allocate_tls_hint(buf->len);
10281003
written = SSL_write(ssl_.get(), buf->base, buf->len);
10291004

10301005
if (written == -1) {
1031-
data = AllocatedBuffer::AllocateManaged(env(), length);
1032-
memcpy(data.data(), buf->base, buf->len);
1006+
NoArrayBufferZeroFillScope no_zero_fill_scope(env()->isolate_data());
1007+
bs = ArrayBuffer::NewBackingStore(env()->isolate(), length);
1008+
memcpy(bs->Data(), buf->base, buf->len);
10331009
}
10341010
}
10351011

10361012
CHECK(written == -1 || written == static_cast<int>(length));
10371013
Debug(this, "Writing %zu bytes, written = %d", length, written);
10381014

10391015
if (written == -1) {
1040-
int err;
1041-
MaybeLocal<Value> arg = GetSSLError(written, &err, &error_);
1042-
10431016
// If we stopped writing because of an error, it's fatal, discard the data.
1044-
if (!arg.IsEmpty()) {
1017+
int err = GetSSLError(written);
1018+
if (err == SSL_ERROR_SSL || err == SSL_ERROR_SYSCALL) {
10451019
// TODO(@jasnell): What are we doing with the error?
10461020
Debug(this, "Got SSL error (%d), returning UV_EPROTO", err);
10471021
current_write_.reset();
@@ -1050,8 +1024,9 @@ int TLSWrap::DoWrite(WriteWrap* w,
10501024

10511025
Debug(this, "Saving data for later write");
10521026
// Otherwise, save unwritten data so it can be written later by ClearIn().
1053-
CHECK_EQ(pending_cleartext_input_.size(), 0);
1054-
pending_cleartext_input_ = std::move(data);
1027+
CHECK(!pending_cleartext_input_ ||
1028+
pending_cleartext_input_->ByteLength() == 0);
1029+
pending_cleartext_input_ = std::move(bs);
10551030
}
10561031

10571032
// Write any encrypted/handshake output that may be ready.
@@ -1492,8 +1467,9 @@ void TLSWrap::MemoryInfo(MemoryTracker* tracker) const {
14921467
tracker->TrackField("sni_context", sni_context_);
14931468
tracker->TrackField("error", error_);
14941469
tracker->TrackFieldWithSize("pending_cleartext_input",
1495-
pending_cleartext_input_.size(),
1496-
"AllocatedBuffer");
1470+
pending_cleartext_input_ ?
1471+
pending_cleartext_input_->ByteLength() : 0,
1472+
"std::unique_ptr<v8::BackingStore>");
14971473
if (enc_in_ != nullptr)
14981474
tracker->TrackField("enc_in", NodeBIO::FromBIO(enc_in_));
14991475
if (enc_out_ != nullptr)

‎src/crypto/crypto_tls.h‎

Lines changed: 2 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -27,7 +27,6 @@
2727
#include "crypto/crypto_context.h"
2828
#include "crypto/crypto_clienthello.h"
2929

30-
#include "allocated_buffer.h"
3130
#include "async_wrap.h"
3231
#include "stream_wrap.h"
3332
#include "v8.h"
@@ -167,7 +166,7 @@ class TLSWrap : public AsyncWrap,
167166

168167
int SetCACerts(SecureContext* sc);
169168

170-
v8::MaybeLocal<v8::Value> GetSSLError(int status, int* err, std::string* msg);
169+
int GetSSLError(int status) const;
171170

172171
static int SelectSNIContextCallback(SSL* s, int* ad, void* arg);
173172

@@ -254,7 +253,7 @@ class TLSWrap : public AsyncWrap,
254253
BIO* enc_in_ = nullptr; // StreamListener fills this for SSL_read().
255254
BIO* enc_out_ = nullptr; // SSL_write()/handshake fills this for EncOut().
256255
// Waiting for ClearIn() to pass to SSL_write().
257-
AllocatedBuffer pending_cleartext_input_;
256+
std::unique_ptr<v8::BackingStore> pending_cleartext_input_;
258257
size_t write_size_ = 0;
259258
BaseObjectPtr<AsyncWrap> current_write_;
260259
BaseObjectPtr<AsyncWrap> current_empty_write_;

0 commit comments

Comments
 (0)