Skip to content

Commit 261229d

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 <[email protected]>
1 parent 47fb867 commit 261229d

File tree

2 files changed

+102
-126
lines changed

2 files changed

+102
-126
lines changed

‎src/crypto/crypto_tls.cc‎

Lines changed: 100 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,62 @@ 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:
795+
return;
796+
}
820797

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();
798+
Debug(this, "Got SSL error (%d), calling onerror", err);
799+
// When TLS Alert are stored in wbio,
800+
// it should be flushed to socket before destroyed.
801+
if (BIO_pending(enc_out_) != 0)
802+
EncOut();
827803

828-
MakeCallback(env()->onerror_string(), 1, &arg);
829-
}
804+
MakeCallback(env()->onerror_string(), 1, &exception);
830805
}
831806
}
832807

@@ -843,18 +818,19 @@ void TLSWrap::ClearIn() {
843818
return;
844819
}
845820

846-
if (pending_cleartext_input_.size() == 0) {
821+
if (!pending_cleartext_input_ ||
822+
pending_cleartext_input_->ByteLength() == 0) {
847823
Debug(this, "Returning from ClearIn(), no pending data");
848824
return;
849825
}
850826

851-
AllocatedBuffer data = std::move(pending_cleartext_input_);
827+
std::unique_ptr<BackingStore> bs = std::move(pending_cleartext_input_);
852828
MarkPopErrorOnReturn mark_pop_error_on_return;
853829

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()));
830+
NodeBIO::FromBIO(enc_out_)->set_allocate_tls_hint(bs->ByteLength());
831+
int written = SSL_write(ssl_.get(), bs->Data(), bs->ByteLength());
832+
Debug(this, "Writing %zu bytes, written = %d", bs->ByteLength(), written);
833+
CHECK(written == -1 || written == static_cast<int>(bs->ByteLength()));
858834

859835
// All written
860836
if (written != -1) {
@@ -863,24 +839,20 @@ void TLSWrap::ClearIn() {
863839
}
864840

865841
// 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()) {
842+
int err = GetSSLError(written);
843+
if (err == SSL_ERROR_SSL || err == SSL_ERROR_SYSCALL) {
873844
Debug(this, "Got SSL error (%d)", err);
874845
write_callback_scheduled_ = true;
875846
// TODO(@sam-github) Should forward an error object with
876847
// .code/.function/.etc, if possible.
877-
return InvokeQueued(UV_EPROTO, error_str.c_str());
848+
InvokeQueued(UV_EPROTO, GetBIOError().c_str());
849+
return;
878850
}
879851

880852
Debug(this, "Pushing data back");
881853
// Push back the not-yet-written data. This can be skipped in the error
882854
// case because no further writes would succeed anyway.
883-
pending_cleartext_input_ = std::move(data);
855+
pending_cleartext_input_ = std::move(bs);
884856
}
885857

886858
std::string TLSWrap::diagnostic_name() const {
@@ -998,7 +970,7 @@ int TLSWrap::DoWrite(WriteWrap* w,
998970
return 0;
999971
}
1000972

1001-
AllocatedBuffer data;
973+
std::unique_ptr<BackingStore> bs;
1002974
MarkPopErrorOnReturn mark_pop_error_on_return;
1003975

1004976
int written = 0;
@@ -1012,36 +984,39 @@ int TLSWrap::DoWrite(WriteWrap* w,
1012984
// and copying it when it could just be used.
1013985

1014986
if (nonempty_count != 1) {
1015-
data = AllocatedBuffer::AllocateManaged(env(), length);
987+
{
988+
NoArrayBufferZeroFillScope no_zero_fill_scope(env()->isolate_data());
989+
bs = ArrayBuffer::NewBackingStore(env()->isolate(), length);
990+
}
1016991
size_t offset = 0;
1017992
for (i = 0; i < count; i++) {
1018-
memcpy(data.data() + offset, bufs[i].base, bufs[i].len);
993+
memcpy(static_cast<char*>(bs->Data()) + offset,
994+
bufs[i].base, bufs[i].len);
1019995
offset += bufs[i].len;
1020996
}
1021997

1022998
NodeBIO::FromBIO(enc_out_)->set_allocate_tls_hint(length);
1023-
written = SSL_write(ssl_.get(), data.data(), length);
999+
written = SSL_write(ssl_.get(), bs->Data(), length);
10241000
} else {
10251001
// Only one buffer: try to write directly, only store if it fails
10261002
uv_buf_t* buf = &bufs[nonempty_i];
10271003
NodeBIO::FromBIO(enc_out_)->set_allocate_tls_hint(buf->len);
10281004
written = SSL_write(ssl_.get(), buf->base, buf->len);
10291005

10301006
if (written == -1) {
1031-
data = AllocatedBuffer::AllocateManaged(env(), length);
1032-
memcpy(data.data(), buf->base, buf->len);
1007+
NoArrayBufferZeroFillScope no_zero_fill_scope(env()->isolate_data());
1008+
bs = ArrayBuffer::NewBackingStore(env()->isolate(), length);
1009+
memcpy(bs->Data(), buf->base, buf->len);
10331010
}
10341011
}
10351012

10361013
CHECK(written == -1 || written == static_cast<int>(length));
10371014
Debug(this, "Writing %zu bytes, written = %d", length, written);
10381015

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

10511026
Debug(this, "Saving data for later write");
10521027
// 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);
1028+
CHECK(!pending_cleartext_input_ ||
1029+
pending_cleartext_input_->ByteLength() == 0);
1030+
pending_cleartext_input_ = std::move(bs);
10551031
}
10561032

10571033
// Write any encrypted/handshake output that may be ready.
@@ -1492,8 +1468,9 @@ void TLSWrap::MemoryInfo(MemoryTracker* tracker) const {
14921468
tracker->TrackField("sni_context", sni_context_);
14931469
tracker->TrackField("error", error_);
14941470
tracker->TrackFieldWithSize("pending_cleartext_input",
1495-
pending_cleartext_input_.size(),
1496-
"AllocatedBuffer");
1471+
pending_cleartext_input_ ?
1472+
pending_cleartext_input_->ByteLength() : 0,
1473+
"std::unique_ptr<v8::BackingStore>");
14971474
if (enc_in_ != nullptr)
14981475
tracker->TrackField("enc_in", NodeBIO::FromBIO(enc_in_));
14991476
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)