From 3ac99a2417570665b828ffbd80c8df5aad0f9dc3 Mon Sep 17 00:00:00 2001 From: Darshan Sen Date: Tue, 12 Oct 2021 15:32:03 +0530 Subject: [PATCH] src,stream: remove `*Check*()` calls from non-`Initialize()` functions There is no need to crash the process if any of these checks fail. Signed-off-by: Darshan Sen PR-URL: https://github.com/nodejs/node/pull/40425 Reviewed-By: Anna Henningsen --- src/stream_base-inl.h | 26 +++++++++------ src/stream_base.cc | 73 +++++++++++++++++++++++++++---------------- src/stream_base.h | 6 ++++ src/stream_pipe.cc | 31 ++++++++++++------ src/stream_pipe.h | 7 ++++- src/stream_wrap.cc | 12 +++---- src/stream_wrap.h | 2 ++ 7 files changed, 104 insertions(+), 53 deletions(-) diff --git a/src/stream_base-inl.h b/src/stream_base-inl.h index 22e5518b589714..ef86587c0bd24e 100644 --- a/src/stream_base-inl.h +++ b/src/stream_base-inl.h @@ -149,9 +149,11 @@ int StreamBase::Shutdown(v8::Local req_wrap_obj) { const char* msg = Error(); if (msg != nullptr) { - req_wrap_obj->Set( - env->context(), - env->error_string(), OneByteString(env->isolate(), msg)).Check(); + if (req_wrap_obj->Set(env->context(), + env->error_string(), + OneByteString(env->isolate(), msg)).IsNothing()) { + return UV_EBUSY; + } ClearError(); } @@ -203,9 +205,11 @@ StreamWriteResult StreamBase::Write( const char* msg = Error(); if (msg != nullptr) { - req_wrap_obj->Set(env->context(), - env->error_string(), - OneByteString(env->isolate(), msg)).Check(); + if (req_wrap_obj->Set(env->context(), + env->error_string(), + OneByteString(env->isolate(), msg)).IsNothing()) { + return StreamWriteResult { false, UV_EBUSY, nullptr, 0, {} }; + } ClearError(); } @@ -279,10 +283,12 @@ void StreamReq::Done(int status, const char* error_str) { Environment* env = async_wrap->env(); if (error_str != nullptr) { v8::HandleScope handle_scope(env->isolate()); - async_wrap->object()->Set(env->context(), - env->error_string(), - OneByteString(env->isolate(), error_str)) - .Check(); + if (async_wrap->object()->Set( + env->context(), + env->error_string(), + OneByteString(env->isolate(), error_str)).IsNothing()) { + return; + } } OnDone(status); diff --git a/src/stream_base.cc b/src/stream_base.cc index 5bef709ef0a559..a47fad970355dc 100644 --- a/src/stream_base.cc +++ b/src/stream_base.cc @@ -106,23 +106,30 @@ int StreamBase::Writev(const FunctionCallbackInfo& args) { if (!all_buffers) { // Determine storage size first for (size_t i = 0; i < count; i++) { - Local chunk = chunks->Get(context, i * 2).ToLocalChecked(); + Local chunk; + if (!chunks->Get(context, i * 2).ToLocal(&chunk)) + return -1; if (Buffer::HasInstance(chunk)) continue; // Buffer chunk, no additional storage required // String chunk - Local string = chunk->ToString(context).ToLocalChecked(); - enum encoding encoding = ParseEncoding(isolate, - chunks->Get(context, i * 2 + 1).ToLocalChecked()); + Local string; + if (!chunk->ToString(context).ToLocal(&string)) + return -1; + Local next_chunk; + if (!chunks->Get(context, i * 2 + 1).ToLocal(&next_chunk)) + return -1; + enum encoding encoding = ParseEncoding(isolate, next_chunk); size_t chunk_size; - if (encoding == UTF8 && string->Length() > 65535 && - !StringBytes::Size(isolate, string, encoding).To(&chunk_size)) - return 0; - else if (!StringBytes::StorageSize(isolate, string, encoding) - .To(&chunk_size)) - return 0; + if ((encoding == UTF8 && + string->Length() > 65535 && + !StringBytes::Size(isolate, string, encoding).To(&chunk_size)) || + !StringBytes::StorageSize(isolate, string, encoding) + .To(&chunk_size)) { + return -1; + } storage_size += chunk_size; } @@ -130,7 +137,9 @@ int StreamBase::Writev(const FunctionCallbackInfo& args) { return UV_ENOBUFS; } else { for (size_t i = 0; i < count; i++) { - Local chunk = chunks->Get(context, i).ToLocalChecked(); + Local chunk; + if (!chunks->Get(context, i).ToLocal(&chunk)) + return -1; bufs[i].base = Buffer::Data(chunk); bufs[i].len = Buffer::Length(chunk); } @@ -145,7 +154,9 @@ int StreamBase::Writev(const FunctionCallbackInfo& args) { offset = 0; if (!all_buffers) { for (size_t i = 0; i < count; i++) { - Local chunk = chunks->Get(context, i * 2).ToLocalChecked(); + Local chunk; + if (!chunks->Get(context, i * 2).ToLocal(&chunk)) + return -1; // Write buffer if (Buffer::HasInstance(chunk)) { @@ -160,9 +171,13 @@ int StreamBase::Writev(const FunctionCallbackInfo& args) { static_cast(bs ? bs->Data() : nullptr) + offset; size_t str_size = (bs ? bs->ByteLength() : 0) - offset; - Local string = chunk->ToString(context).ToLocalChecked(); - enum encoding encoding = ParseEncoding(isolate, - chunks->Get(context, i * 2 + 1).ToLocalChecked()); + Local string; + if (!chunk->ToString(context).ToLocal(&string)) + return -1; + Local next_chunk; + if (!chunks->Get(context, i * 2 + 1).ToLocal(&next_chunk)) + return -1; + enum encoding encoding = ParseEncoding(isolate, next_chunk); str_size = StringBytes::Write(isolate, str_storage, str_size, @@ -207,9 +222,11 @@ int StreamBase::WriteBuffer(const FunctionCallbackInfo& args) { send_handle = reinterpret_cast(wrap->GetHandle()); // Reference LibuvStreamWrap instance to prevent it from being garbage // collected before `AfterWrite` is called. - req_wrap_obj->Set(env->context(), - env->handle_string(), - send_handle_obj).Check(); + if (req_wrap_obj->Set(env->context(), + env->handle_string(), + send_handle_obj).IsNothing()) { + return -1; + } } StreamWriteResult res = Write(&buf, 1, send_handle, req_wrap_obj); @@ -236,12 +253,12 @@ int StreamBase::WriteString(const FunctionCallbackInfo& args) { // For UTF8 strings that are very long, go ahead and take the hit for // computing their actual size, rather than tripling the storage. size_t storage_size; - if (enc == UTF8 && string->Length() > 65535 && - !StringBytes::Size(isolate, string, enc).To(&storage_size)) - return 0; - else if (!StringBytes::StorageSize(isolate, string, enc) - .To(&storage_size)) - return 0; + if ((enc == UTF8 && + string->Length() > 65535 && + !StringBytes::Size(isolate, string, enc).To(&storage_size)) || + !StringBytes::StorageSize(isolate, string, enc).To(&storage_size)) { + return -1; + } if (storage_size > INT_MAX) return UV_ENOBUFS; @@ -312,9 +329,11 @@ int StreamBase::WriteString(const FunctionCallbackInfo& args) { send_handle = reinterpret_cast(wrap->GetHandle()); // Reference LibuvStreamWrap instance to prevent it from being garbage // collected before `AfterWrite` is called. - req_wrap_obj->Set(env->context(), - env->handle_string(), - send_handle_obj).Check(); + if (req_wrap_obj->Set(env->context(), + env->handle_string(), + send_handle_obj).IsNothing()) { + return -1; + } } StreamWriteResult res = Write(&buf, 1, send_handle, req_wrap_obj); diff --git a/src/stream_base.h b/src/stream_base.h index 019048fbbd90ba..7513225362d3ec 100644 --- a/src/stream_base.h +++ b/src/stream_base.h @@ -49,6 +49,8 @@ class StreamReq { virtual AsyncWrap* GetAsyncWrap() = 0; inline v8::Local object(); + // TODO(RaisinTen): Update the return type to a Maybe, so that we can indicate + // if there is a pending exception/termination. inline void Done(int status, const char* error_str = nullptr); inline void Dispose(); @@ -326,6 +328,8 @@ class StreamBase : public StreamResource { // subclasses are also `BaseObject`s. Environment* stream_env() const { return env_; } + // TODO(RaisinTen): Update the return type to a Maybe, so that we can indicate + // if there is a pending exception/termination. // Shut down the current stream. This request can use an existing // ShutdownWrap object (that was created in JS), or a new one will be created. // Returns 1 in case of a synchronous completion, 0 in case of asynchronous @@ -333,6 +337,8 @@ class StreamBase : public StreamResource { inline int Shutdown( v8::Local req_wrap_obj = v8::Local()); + // TODO(RaisinTen): Update the return type to a Maybe, so that we can indicate + // if there is a pending exception/termination. // Write data to the current stream. This request can use an existing // WriteWrap object (that was created in JS), or a new one will be created. // This will first try to write synchronously using `DoTryWrite()`, then diff --git a/src/stream_pipe.cc b/src/stream_pipe.cc index aa636dbb75e9ed..94ba8604bd76c6 100644 --- a/src/stream_pipe.cc +++ b/src/stream_pipe.cc @@ -33,14 +33,26 @@ StreamPipe::StreamPipe(StreamBase* source, // In particular, this makes sure that they are garbage collected as a group, // if that applies to the given streams (for example, Http2Streams use // weak references). - obj->Set(env()->context(), env()->source_string(), source->GetObject()) - .Check(); - source->GetObject()->Set(env()->context(), env()->pipe_target_string(), obj) - .Check(); - obj->Set(env()->context(), env()->sink_string(), sink->GetObject()) - .Check(); - sink->GetObject()->Set(env()->context(), env()->pipe_source_string(), obj) - .Check(); + if (obj->Set(env()->context(), + env()->source_string(), + source->GetObject()).IsNothing()) { + return; + } + if (source->GetObject()->Set(env()->context(), + env()->pipe_target_string(), + obj).IsNothing()) { + return; + } + if (obj->Set(env()->context(), + env()->sink_string(), + sink->GetObject()).IsNothing()) { + return; + } + if (sink->GetObject()->Set(env()->context(), + env()->pipe_source_string(), + obj).IsNothing()) { + return; + } } StreamPipe::~StreamPipe() { @@ -172,7 +184,8 @@ void StreamPipe::WritableListener::OnStreamAfterWrite(WriteWrap* w, Environment* env = pipe->env(); HandleScope handle_scope(env->isolate()); Context::Scope context_scope(env->context()); - pipe->MakeCallback(env->oncomplete_string(), 0, nullptr).ToLocalChecked(); + if (pipe->MakeCallback(env->oncomplete_string(), 0, nullptr).IsEmpty()) + return; stream()->RemoveStreamListener(this); } return; diff --git a/src/stream_pipe.h b/src/stream_pipe.h index eb2e914e793175..3c3768602b3e26 100644 --- a/src/stream_pipe.h +++ b/src/stream_pipe.h @@ -9,11 +9,14 @@ namespace node { class StreamPipe : public AsyncWrap { public: - StreamPipe(StreamBase* source, StreamBase* sink, v8::Local obj); ~StreamPipe() override; void Unpipe(bool is_in_deletion = false); + // TODO(RaisinTen): Just like MessagePort, add the following overload: + // static StreamPipe* New(StreamBase* source, StreamBase* sink, + // v8::Local obj); + // so that we can indicate if there is a pending exception/termination. static void New(const v8::FunctionCallbackInfo& args); static void Start(const v8::FunctionCallbackInfo& args); static void Unpipe(const v8::FunctionCallbackInfo& args); @@ -25,6 +28,8 @@ class StreamPipe : public AsyncWrap { SET_SELF_SIZE(StreamPipe) private: + StreamPipe(StreamBase* source, StreamBase* sink, v8::Local obj); + inline StreamBase* source(); inline StreamBase* sink(); diff --git a/src/stream_wrap.cc b/src/stream_wrap.cc index 2275167ad0eeb6..ff3df6bc5db7bd 100644 --- a/src/stream_wrap.cc +++ b/src/stream_wrap.cc @@ -268,12 +268,12 @@ void LibuvStreamWrap::OnUvRead(ssize_t nread, const uv_buf_t* buf) { CHECK_EQ(type, UV_UNKNOWN_HANDLE); } - if (!pending_obj.IsEmpty()) { - object() - ->Set(env()->context(), - env()->pending_handle_string(), - pending_obj.ToLocalChecked()) - .Check(); + Local local_pending_obj; + if (pending_obj.ToLocal(&local_pending_obj) && + object()->Set(env()->context(), + env()->pending_handle_string(), + local_pending_obj).IsNothing()) { + return; } } diff --git a/src/stream_wrap.h b/src/stream_wrap.h index 3016e8d89cd367..8cbce923bfed89 100644 --- a/src/stream_wrap.h +++ b/src/stream_wrap.h @@ -105,6 +105,8 @@ class LibuvStreamWrap : public HandleWrap, public StreamBase { // Callbacks for libuv void OnUvAlloc(size_t suggested_size, uv_buf_t* buf); + // TODO(RaisinTen): Update the return type to a Maybe, so that we can indicate + // if there is a pending exception/termination. void OnUvRead(ssize_t nread, const uv_buf_t* buf); static void AfterUvWrite(uv_write_t* req, int status);