From d6f3b8785fcd5e4700f851f376a8f17861a7971f Mon Sep 17 00:00:00 2001 From: Anna Henningsen Date: Wed, 30 Jan 2019 19:11:11 +0100 Subject: [PATCH] src: pass along errors from fs object creations PR-URL: https://github.com/nodejs/node/pull/25822 Reviewed-By: Gireesh Punathil --- 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); }