From f8ea8fa5ac4b2a1a5c4453ccebf56ffad5a18bdd Mon Sep 17 00:00:00 2001 From: Anna Henningsen Date: Wed, 30 Jan 2019 18:42:25 +0100 Subject: [PATCH 1/4] src: pass along errors from tls object creation --- src/tls_wrap.cc | 15 ++++++++++----- src/tls_wrap.h | 1 + 2 files changed, 11 insertions(+), 5 deletions(-) diff --git a/src/tls_wrap.cc b/src/tls_wrap.cc index e467e2d167f628..8bcd0c849bafd2 100644 --- a/src/tls_wrap.cc +++ b/src/tls_wrap.cc @@ -49,13 +49,11 @@ using v8::String; using v8::Value; TLSWrap::TLSWrap(Environment* env, + Local obj, Kind kind, StreamBase* stream, SecureContext* sc) - : AsyncWrap(env, - env->tls_wrap_constructor_function() - ->NewInstance(env->context()).ToLocalChecked(), - AsyncWrap::PROVIDER_TLSWRAP), + : AsyncWrap(env, obj, AsyncWrap::PROVIDER_TLSWRAP), SSLWrap(env, sc, kind), StreamBase(env), sc_(sc) { @@ -160,7 +158,14 @@ void TLSWrap::Wrap(const FunctionCallbackInfo& args) { StreamBase* stream = static_cast(stream_obj->Value()); CHECK_NOT_NULL(stream); - TLSWrap* res = new TLSWrap(env, kind, stream, Unwrap(sc)); + Local obj; + if (!env->tls_wrap_constructor_function() + ->NewInstance(env->context()) + .ToLocal(&obj)) { + return; + } + + TLSWrap* res = new TLSWrap(env, obj, kind, stream, Unwrap(sc)); args.GetReturnValue().Set(res->object()); } diff --git a/src/tls_wrap.h b/src/tls_wrap.h index cd2701cd6d9fb7..5c3a7e3fb9b1de 100644 --- a/src/tls_wrap.h +++ b/src/tls_wrap.h @@ -101,6 +101,7 @@ class TLSWrap : public AsyncWrap, static const int kSimultaneousBufferCount = 10; TLSWrap(Environment* env, + v8::Local obj, Kind kind, StreamBase* stream, crypto::SecureContext* sc); From 93d39c33477ecac3d476290051f8acc3bf7a9f03 Mon Sep 17 00:00:00 2001 From: Anna Henningsen Date: Wed, 30 Jan 2019 18:43:05 +0100 Subject: [PATCH 2/4] src: pass along errors from http2 object creation --- src/node_http2.cc | 125 ++++++++++++++++++++++++++-------------------- src/node_http2.h | 24 ++++++--- 2 files changed, 87 insertions(+), 62 deletions(-) diff --git a/src/node_http2.cc b/src/node_http2.cc index 5b172dfee78e87..0297dcc9d21894 100644 --- a/src/node_http2.cc +++ b/src/node_http2.cc @@ -228,27 +228,18 @@ void Http2Session::Http2Settings::Init() { count_ = n; } -Http2Session::Http2Settings::Http2Settings(Environment* env, - Http2Session* session, uint64_t start_time) - : AsyncWrap(env, - env->http2settings_constructor_template() - ->NewInstance(env->context()) - .ToLocalChecked(), - PROVIDER_HTTP2SETTINGS), - session_(session), - startTime_(start_time) { - Init(); -} - - -Http2Session::Http2Settings::Http2Settings(Environment* env) - : Http2Settings(env, nullptr, 0) {} - // The Http2Settings class is used to configure a SETTINGS frame that is // to be sent to the connected peer. The settings are set using a TypedArray // that is shared with the JavaScript side. -Http2Session::Http2Settings::Http2Settings(Http2Session* session) - : Http2Settings(session->env(), session, uv_hrtime()) {} +Http2Session::Http2Settings::Http2Settings(Environment* env, + Http2Session* session, + Local obj, + uint64_t start_time) + : AsyncWrap(env, obj, PROVIDER_HTTP2SETTINGS), + session_(session), + startTime_(start_time) { + Init(); +} // Generates a Buffer that contains the serialized payload of a SETTINGS // frame. This can be used, for instance, to create the Base64-encoded @@ -918,13 +909,14 @@ int Http2Session::OnBeginHeadersCallback(nghttp2_session* handle, // The common case is that we're creating a new stream. The less likely // case is that we're receiving a set of trailers if (LIKELY(stream == nullptr)) { - if (UNLIKELY(!session->CanAddStream())) { + if (UNLIKELY(!session->CanAddStream() || + Http2Stream::New(session, id, frame->headers.cat) == + nullptr)) { // Too many concurrent streams being opened nghttp2_submit_rst_stream(**session, NGHTTP2_FLAG_NONE, id, NGHTTP2_ENHANCE_YOUR_CALM); return NGHTTP2_ERR_TEMPORAL_CALLBACK_FAILURE; } - new Http2Stream(session, id, frame->headers.cat); } else if (!stream->IsDestroyed()) { stream->StartHeaders(frame->headers.cat); } @@ -1771,7 +1763,7 @@ Http2Stream* Http2Session::SubmitRequest( *ret = nghttp2_submit_request(session_, prispec, nva, len, *prov, nullptr); CHECK_NE(*ret, NGHTTP2_ERR_NOMEM); if (LIKELY(*ret > 0)) - stream = new Http2Stream(this, *ret, NGHTTP2_HCAT_HEADERS, options); + stream = Http2Stream::New(this, *ret, NGHTTP2_HCAT_HEADERS, options); return stream; } @@ -1857,20 +1849,30 @@ void Http2Session::Consume(Local external) { Debug(this, "i/o stream consumed"); } - -Http2Stream::Http2Stream( - Http2Session* session, - int32_t id, - nghttp2_headers_category category, - int options) : AsyncWrap(session->env(), - session->env()->http2stream_constructor_template() - ->NewInstance(session->env()->context()) - .ToLocalChecked(), - AsyncWrap::PROVIDER_HTTP2STREAM), - StreamBase(session->env()), - session_(session), - id_(id), - current_headers_category_(category) { +Http2Stream* Http2Stream::New(Http2Session* session, + int32_t id, + nghttp2_headers_category category, + int options) { + Local obj; + if (!session->env() + ->http2stream_constructor_template() + ->NewInstance(session->env()->context()) + .ToLocal(&obj)) { + return nullptr; + } + return new Http2Stream(session, obj, id, category, options); +} + +Http2Stream::Http2Stream(Http2Session* session, + Local obj, + int32_t id, + nghttp2_headers_category category, + int options) + : AsyncWrap(session->env(), obj, AsyncWrap::PROVIDER_HTTP2STREAM), + StreamBase(session->env()), + session_(session), + id_(id), + current_headers_category_(category) { MakeWeak(); statistics_.start_time = uv_hrtime(); @@ -2113,7 +2115,7 @@ Http2Stream* Http2Stream::SubmitPushPromise(nghttp2_nv* nva, CHECK_NE(*ret, NGHTTP2_ERR_NOMEM); Http2Stream* stream = nullptr; if (*ret > 0) - stream = new Http2Stream(session_, *ret, NGHTTP2_HCAT_HEADERS, options); + stream = Http2Stream::New(session_, *ret, NGHTTP2_HCAT_HEADERS, options); return stream; } @@ -2335,7 +2337,14 @@ void HttpErrorString(const FunctionCallbackInfo& args) { // output for an HTTP2-Settings header field. void PackSettings(const FunctionCallbackInfo& args) { Environment* env = Environment::GetCurrent(args); - Http2Session::Http2Settings settings(env); + // TODO(addaleax): We should not be creating a full AsyncWrap for this. + Local obj; + if (!env->http2settings_constructor_template() + ->NewInstance(env->context()) + .ToLocal(&obj)) { + return; + } + Http2Session::Http2Settings settings(env, nullptr, obj); args.GetReturnValue().Set(settings.Pack()); } @@ -2464,7 +2473,7 @@ void Http2Session::Request(const FunctionCallbackInfo& args) { session->Http2Session::SubmitRequest(*priority, *list, list.length(), &ret, options); - if (ret <= 0) { + if (ret <= 0 || stream == nullptr) { Debug(session, "could not submit request: %s", nghttp2_strerror(ret)); return args.GetReturnValue().Set(ret); } @@ -2637,7 +2646,7 @@ void Http2Stream::PushPromise(const FunctionCallbackInfo& args) { int32_t ret = 0; Http2Stream* stream = parent->SubmitPushPromise(*list, list.length(), &ret, options); - if (ret <= 0) { + if (ret <= 0 || stream == nullptr) { Debug(parent, "failed to create push stream: %d", ret); return args.GetReturnValue().Set(ret); } @@ -2772,9 +2781,15 @@ void Http2Session::Ping(const FunctionCallbackInfo& args) { CHECK_EQ(Buffer::Length(args[0]), 8); } - Http2Session::Http2Ping* ping = new Http2Ping(session); - Local obj = ping->object(); - obj->Set(env->context(), env->ondone_string(), args[1]).FromJust(); + Local obj; + if (!env->http2ping_constructor_template() + ->NewInstance(env->context()) + .ToLocal(&obj)) { + return; + } + if (obj->Set(env->context(), env->ondone_string(), args[1]).IsNothing()) + return; + Http2Session::Http2Ping* ping = new Http2Ping(session, obj); // To prevent abuse, we strictly limit the number of unacknowledged PING // frames that may be sent at any given time. This is configurable in the @@ -2798,10 +2813,17 @@ void Http2Session::Settings(const FunctionCallbackInfo& args) { Http2Session* session; ASSIGN_OR_RETURN_UNWRAP(&session, args.Holder()); - Http2Session::Http2Settings* settings = new Http2Settings(session); - Local obj = settings->object(); - obj->Set(env->context(), env->ondone_string(), args[0]).FromJust(); + Local obj; + if (!env->http2settings_constructor_template() + ->NewInstance(env->context()) + .ToLocal(&obj)) { + return; + } + if (obj->Set(env->context(), env->ondone_string(), args[0]).IsNothing()) + return; + Http2Session::Http2Settings* settings = + new Http2Settings(session->env(), session, obj, 0); if (!session->AddSettings(settings)) { settings->Done(false); return args.GetReturnValue().Set(false); @@ -2848,15 +2870,10 @@ bool Http2Session::AddSettings(Http2Session::Http2Settings* settings) { return true; } -Http2Session::Http2Ping::Http2Ping( - Http2Session* session) - : AsyncWrap(session->env(), - session->env()->http2ping_constructor_template() - ->NewInstance(session->env()->context()) - .ToLocalChecked(), - AsyncWrap::PROVIDER_HTTP2PING), - session_(session), - startTime_(uv_hrtime()) { } +Http2Session::Http2Ping::Http2Ping(Http2Session* session, Local obj) + : AsyncWrap(session->env(), obj, AsyncWrap::PROVIDER_HTTP2PING), + session_(session), + startTime_(uv_hrtime()) {} void Http2Session::Http2Ping::Send(uint8_t* payload) { uint8_t data[8]; diff --git a/src/node_http2.h b/src/node_http2.h index 4446bf40787371..994f9a6b57f42f 100644 --- a/src/node_http2.h +++ b/src/node_http2.h @@ -451,10 +451,11 @@ class Http2StreamListener : public StreamListener { class Http2Stream : public AsyncWrap, public StreamBase { public: - Http2Stream(Http2Session* session, - int32_t id, - nghttp2_headers_category category = NGHTTP2_HCAT_HEADERS, - int options = 0); + static Http2Stream* New( + Http2Session* session, + int32_t id, + nghttp2_headers_category category = NGHTTP2_HCAT_HEADERS, + int options = 0); ~Http2Stream() override; nghttp2_stream* operator*(); @@ -611,6 +612,12 @@ class Http2Stream : public AsyncWrap, Statistics statistics_ = {}; private: + Http2Stream(Http2Session* session, + v8::Local obj, + int32_t id, + nghttp2_headers_category category, + int options); + Http2Session* session_ = nullptr; // The Parent HTTP/2 Session int32_t id_ = 0; // The Stream Identifier int32_t code_ = NGHTTP2_NO_ERROR; // The RST_STREAM code (if any) @@ -1087,7 +1094,7 @@ class Http2StreamPerformanceEntry : public PerformanceEntry { class Http2Session::Http2Ping : public AsyncWrap { public: - explicit Http2Ping(Http2Session* session); + explicit Http2Ping(Http2Session* session, v8::Local obj); void MemoryInfo(MemoryTracker* tracker) const override { tracker->TrackField("session", session_); @@ -1111,8 +1118,10 @@ class Http2Session::Http2Ping : public AsyncWrap { // structs. class Http2Session::Http2Settings : public AsyncWrap { public: - explicit Http2Settings(Environment* env); - explicit Http2Settings(Http2Session* session); + Http2Settings(Environment* env, + Http2Session* session, + v8::Local obj, + uint64_t start_time = uv_hrtime()); void MemoryInfo(MemoryTracker* tracker) const override { tracker->TrackField("session", session_); @@ -1136,7 +1145,6 @@ class Http2Session::Http2Settings : public AsyncWrap { get_setting fn); private: - Http2Settings(Environment* env, Http2Session* session, uint64_t start_time); void Init(); Http2Session* session_; uint64_t startTime_; From 0c37ff5907d9ffbbd24917484c612fd517c336e4 Mon Sep 17 00:00:00 2001 From: Anna Henningsen Date: Wed, 30 Jan 2019 19:11:11 +0100 Subject: [PATCH 3/4] src: pass along errors from fs object creations --- src/node_file.cc | 66 ++++++++++++++++++++++++++++++++++-------------- src/node_file.h | 42 +++++++++++++++++------------- 2 files changed, 71 insertions(+), 37 deletions(-) diff --git a/src/node_file.cc b/src/node_file.cc index 35c8e01a28c0b5..5513be3d7c77c1 100644 --- a/src/node_file.cc +++ b/src/node_file.cc @@ -110,20 +110,29 @@ typedef void(*uv_fs_callback_t)(uv_fs_t*); // The FileHandle object wraps a file descriptor and will close it on garbage // collection if necessary. If that happens, a process warning will be // emitted (or a fatal exception will occur if the fd cannot be closed.) -FileHandle::FileHandle(Environment* env, int fd, Local obj) - : AsyncWrap(env, - obj.IsEmpty() ? env->fd_constructor_template() - ->NewInstance(env->context()).ToLocalChecked() : obj, - AsyncWrap::PROVIDER_FILEHANDLE), +FileHandle::FileHandle(Environment* env, Local obj, int fd) + : AsyncWrap(env, obj, AsyncWrap::PROVIDER_FILEHANDLE), StreamBase(env), fd_(fd) { MakeWeak(); +} + +FileHandle* FileHandle::New(Environment* env, int fd, Local obj) { + if (obj.IsEmpty() && !env->fd_constructor_template() + ->NewInstance(env->context()) + .ToLocal(&obj)) { + return nullptr; + } v8::PropertyAttribute attr = static_cast(v8::ReadOnly | v8::DontDelete); - object()->DefineOwnProperty(env->context(), - FIXED_ONE_BYTE_STRING(env->isolate(), "fd"), - Integer::New(env->isolate(), fd), - attr).FromJust(); + if (obj->DefineOwnProperty(env->context(), + env->fd_string(), + Integer::New(env->isolate(), fd), + attr) + .IsNothing()) { + return nullptr; + } + return new FileHandle(env, obj, fd); } void FileHandle::New(const FunctionCallbackInfo& args) { @@ -132,7 +141,8 @@ void FileHandle::New(const FunctionCallbackInfo& args) { CHECK(args[0]->IsInt32()); FileHandle* handle = - new FileHandle(env, args[0].As()->Value(), args.This()); + FileHandle::New(env, args[0].As()->Value(), args.This()); + if (handle == nullptr) return; if (args[1]->IsNumber()) handle->read_offset_ = args[1]->IntegerValue(env->context()).FromJust(); if (args[2]->IsNumber()) @@ -232,7 +242,14 @@ inline MaybeLocal FileHandle::ClosePromise() { CHECK(!reading_); if (!closed_ && !closing_) { closing_ = true; - CloseReq* req = new CloseReq(env(), promise, object()); + Local close_req_obj; + if (!env() + ->fdclose_constructor_template() + ->NewInstance(env()->context()) + .ToLocal(&close_req_obj)) { + return MaybeLocal(); + } + CloseReq* req = new CloseReq(env(), close_req_obj, promise, object()); auto AfterClose = uv_fs_callback_t{[](uv_fs_t* req) { std::unique_ptr close(CloseReq::from_req(req)); CHECK_NOT_NULL(close); @@ -260,7 +277,9 @@ inline MaybeLocal FileHandle::ClosePromise() { void FileHandle::Close(const FunctionCallbackInfo& args) { FileHandle* fd; ASSIGN_OR_RETURN_UNWRAP(&fd, args.Holder()); - args.GetReturnValue().Set(fd->ClosePromise().ToLocalChecked()); + Local ret; + if (!fd->ClosePromise().ToLocal(&ret)) return; + args.GetReturnValue().Set(ret); } @@ -318,8 +337,13 @@ int FileHandle::ReadStart() { read_wrap->AsyncReset(); read_wrap->file_handle_ = this; } else { - Local wrap_obj = env()->filehandlereadwrap_template() - ->NewInstance(env()->context()).ToLocalChecked(); + Local wrap_obj; + if (!env() + ->filehandlereadwrap_template() + ->NewInstance(env()->context()) + .ToLocal(&wrap_obj)) { + return UV_EBUSY; + } read_wrap.reset(new FileHandleReadWrap(this, wrap_obj)); } } @@ -520,7 +544,8 @@ void AfterOpenFileHandle(uv_fs_t* req) { FSReqAfterScope after(req_wrap, req); if (after.Proceed()) { - FileHandle* fd = new FileHandle(req_wrap->env(), req->result); + FileHandle* fd = FileHandle::New(req_wrap->env(), req->result); + if (fd == nullptr) return; req_wrap->Resolve(fd->object()); } } @@ -724,15 +749,18 @@ inline int SyncCall(Environment* env, Local ctx, FSReqWrapSync* req_wrap, return err; } +// TODO(addaleax): Currently, callers check the return value and assume +// that nullptr indicates a synchronous call, rather than a failure. +// Failure conditions should be disambiguated and handled appropriately. inline FSReqBase* GetReqWrap(Environment* env, Local value, bool use_bigint = false) { if (value->IsObject()) { return Unwrap(value.As()); } else if (value->StrictEquals(env->fs_use_promises_symbol())) { if (use_bigint) { - return new FSReqPromise(env, use_bigint); + return FSReqPromise::New(env, use_bigint); } else { - return new FSReqPromise(env, use_bigint); + return FSReqPromise::New(env, use_bigint); } } return nullptr; @@ -1562,8 +1590,8 @@ static void OpenFileHandle(const FunctionCallbackInfo& args) { if (result < 0) { return; // syscall failed, no need to continue, error info is in ctx } - HandleScope scope(isolate); - FileHandle* fd = new FileHandle(env, result); + FileHandle* fd = FileHandle::New(env, result); + if (fd == nullptr) return; args.GetReturnValue().Set(fd->object()); } } diff --git a/src/node_file.h b/src/node_file.h index e2f8bdc55e6ad1..5ae01df9ef6106 100644 --- a/src/node_file.h +++ b/src/node_file.h @@ -237,17 +237,19 @@ inline Local FillGlobalStatsArray(Environment* env, template class FSReqPromise : public FSReqBase { public: - explicit FSReqPromise(Environment* env, bool use_bigint) - : FSReqBase(env, - env->fsreqpromise_constructor_template() - ->NewInstance(env->context()).ToLocalChecked(), - AsyncWrap::PROVIDER_FSREQPROMISE, - use_bigint), - stats_field_array_(env->isolate(), kFsStatsFieldsNumber) { - const auto resolver = - Promise::Resolver::New(env->context()).ToLocalChecked(); - USE(object()->Set(env->context(), env->promise_string(), - resolver).FromJust()); + static FSReqPromise* New(Environment* env, bool use_bigint) { + v8::Local obj; + if (!env->fsreqpromise_constructor_template() + ->NewInstance(env->context()) + .ToLocal(&obj)) { + return nullptr; + } + v8::Local resolver; + if (!v8::Promise::Resolver::New(env->context()).ToLocal(&resolver) || + obj->Set(env->context(), env->promise_string(), resolver).IsNothing()) { + return nullptr; + } + return new FSReqPromise(env, obj, use_bigint); } ~FSReqPromise() override { @@ -304,6 +306,10 @@ class FSReqPromise : public FSReqBase { FSReqPromise& operator=(const FSReqPromise&&) = delete; private: + FSReqPromise(Environment* env, v8::Local obj, bool use_bigint) + : FSReqBase(env, obj, AsyncWrap::PROVIDER_FSREQPROMISE, use_bigint), + stats_field_array_(env->isolate(), kFsStatsFieldsNumber) {} + bool finished_ = false; AliasedBuffer stats_field_array_; }; @@ -356,9 +362,9 @@ class FileHandleReadWrap : public ReqWrap { // the object is garbage collected class FileHandle : public AsyncWrap, public StreamBase { public: - FileHandle(Environment* env, - int fd, - v8::Local obj = v8::Local()); + static FileHandle* New(Environment* env, + int fd, + v8::Local obj = v8::Local()); virtual ~FileHandle(); static void New(const v8::FunctionCallbackInfo& args); @@ -404,6 +410,8 @@ class FileHandle : public AsyncWrap, public StreamBase { FileHandle& operator=(const FileHandle&&) = delete; private: + FileHandle(Environment* env, v8::Local obj, int fd); + // Synchronous close that emits a warning void Close(); void AfterClose(); @@ -411,12 +419,10 @@ class FileHandle : public AsyncWrap, public StreamBase { class CloseReq : public ReqWrap { public: CloseReq(Environment* env, + Local obj, Local promise, Local ref) - : ReqWrap(env, - env->fdclose_constructor_template() - ->NewInstance(env->context()).ToLocalChecked(), - AsyncWrap::PROVIDER_FILEHANDLECLOSEREQ) { + : ReqWrap(env, obj, AsyncWrap::PROVIDER_FILEHANDLECLOSEREQ) { promise_.Reset(env->isolate(), promise); ref_.Reset(env->isolate(), ref); } From ce8dcae3a540cb647f183a2cafeceff2a7c9f16c Mon Sep 17 00:00:00 2001 From: Anna Henningsen Date: Wed, 30 Jan 2019 19:15:58 +0100 Subject: [PATCH 4/4] src: pass along errors from StreamBase req obj creations --- src/stream_base-inl.h | 16 ++++++++++------ 1 file changed, 10 insertions(+), 6 deletions(-) diff --git a/src/stream_base-inl.h b/src/stream_base-inl.h index 7e2bbaa1730f2e..7db8403ced832b 100644 --- a/src/stream_base-inl.h +++ b/src/stream_base-inl.h @@ -163,9 +163,11 @@ inline int StreamBase::Shutdown(v8::Local req_wrap_obj) { HandleScope handle_scope(env->isolate()); if (req_wrap_obj.IsEmpty()) { - req_wrap_obj = - env->shutdown_wrap_template() - ->NewInstance(env->context()).ToLocalChecked(); + if (!env->shutdown_wrap_template() + ->NewInstance(env->context()) + .ToLocal(&req_wrap_obj)) { + return UV_EBUSY; + } StreamReq::ResetObject(req_wrap_obj); } @@ -211,9 +213,11 @@ inline StreamWriteResult StreamBase::Write( HandleScope handle_scope(env->isolate()); if (req_wrap_obj.IsEmpty()) { - req_wrap_obj = - env->write_wrap_template() - ->NewInstance(env->context()).ToLocalChecked(); + if (!env->write_wrap_template() + ->NewInstance(env->context()) + .ToLocal(&req_wrap_obj)) { + return StreamWriteResult { false, UV_EBUSY, nullptr, 0 }; + } StreamReq::ResetObject(req_wrap_obj); }