src: pass along errors from fs object creations

PR-URL: #25822 Reviewed-By: Gireesh Punathil <gpunathi@in.ibm.com>
nodejs · Feb 8, 2019 · d6f3b8785fcd5e4700f851f376a8f17861a7971f · d6f3b87
1 parent 0672c24
commit d6f3b8785fcd5e4700f851f376a8f17861a7971f
Showing with 71 additions and 37 deletions.

+47 −19 src/node_file.cc

+24 −18 src/node_file.h
diff --git 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<Object> obj)
-    : AsyncWrap(env,
-                obj.IsEmpty() ? env->fd_constructor_template()
-                    ->NewInstance(env->context()).ToLocalChecked() : obj,
-                AsyncWrap::PROVIDER_FILEHANDLE),
+FileHandle::FileHandle(Environment* env, Local<Object> obj, int fd)
+    : AsyncWrap(env, obj, AsyncWrap::PROVIDER_FILEHANDLE),
       StreamBase(env),
       fd_(fd) {
   MakeWeak();
+}
+
+FileHandle* FileHandle::New(Environment* env, int fd, Local<Object> obj) {
+  if (obj.IsEmpty() && !env->fd_constructor_template()
+                            ->NewInstance(env->context())
+                            .ToLocal(&obj)) {
+    return nullptr;
+  }
   v8::PropertyAttribute attr =
       static_cast<v8::PropertyAttribute>(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<Value>& args) {
@@ -132,7 +141,8 @@ void FileHandle::New(const FunctionCallbackInfo<Value>& args) {
   CHECK(args[0]->IsInt32());
 
   FileHandle* handle =
-      new FileHandle(env, args[0].As<Int32>()->Value(), args.This());
+      FileHandle::New(env, args[0].As<Int32>()->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<Promise> FileHandle::ClosePromise() {
   CHECK(!reading_);
   if (!closed_ && !closing_) {
     closing_ = true;
-    CloseReq* req = new CloseReq(env(), promise, object());
+    Local<Object> close_req_obj;
+    if (!env()
+             ->fdclose_constructor_template()
+             ->NewInstance(env()->context())
+             .ToLocal(&close_req_obj)) {
+      return MaybeLocal<Promise>();
+    }
+    CloseReq* req = new CloseReq(env(), close_req_obj, promise, object());
     auto AfterClose = uv_fs_callback_t{[](uv_fs_t* req) {
       std::unique_ptr<CloseReq> close(CloseReq::from_req(req));
       CHECK_NOT_NULL(close);
@@ -260,7 +277,9 @@ inline MaybeLocal<Promise> FileHandle::ClosePromise() {
 void FileHandle::Close(const FunctionCallbackInfo<Value>& args) {
   FileHandle* fd;
   ASSIGN_OR_RETURN_UNWRAP(&fd, args.Holder());
-  args.GetReturnValue().Set(fd->ClosePromise().ToLocalChecked());
+  Local<Promise> 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<Object> wrap_obj = env()->filehandlereadwrap_template()
-          ->NewInstance(env()->context()).ToLocalChecked();
+      Local<Object> 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<Value> 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> value,
                              bool use_bigint = false) {
   if (value->IsObject()) {
     return Unwrap<FSReqBase>(value.As<Object>());
   } else if (value->StrictEquals(env->fs_use_promises_symbol())) {
     if (use_bigint) {
-      return new FSReqPromise<uint64_t, BigUint64Array>(env, use_bigint);
+      return FSReqPromise<uint64_t, BigUint64Array>::New(env, use_bigint);
     } else {
-      return new FSReqPromise<double, Float64Array>(env, use_bigint);
+      return FSReqPromise<double, Float64Array>::New(env, use_bigint);
     }
   }
   return nullptr;
@@ -1562,8 +1590,8 @@ static void OpenFileHandle(const FunctionCallbackInfo<Value>& 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
@@ -237,17 +237,19 @@ inline Local<Value> FillGlobalStatsArray(Environment* env,
 template <typename NativeT = double, typename V8T = v8::Float64Array>
 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<Object> obj;
+    if (!env->fsreqpromise_constructor_template()
+             ->NewInstance(env->context())
+             .ToLocal(&obj)) {
+      return nullptr;
+    }
+    v8::Local<v8::Promise::Resolver> 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<v8::Object> obj, bool use_bigint)
+      : FSReqBase(env, obj, AsyncWrap::PROVIDER_FSREQPROMISE, use_bigint),
+        stats_field_array_(env->isolate(), kFsStatsFieldsNumber) {}
+
   bool finished_ = false;
   AliasedBuffer<NativeT, V8T> stats_field_array_;
 };
@@ -356,9 +362,9 @@ class FileHandleReadWrap : public ReqWrap<uv_fs_t> {
 // the object is garbage collected
 class FileHandle : public AsyncWrap, public StreamBase {
  public:
-  FileHandle(Environment* env,
-             int fd,
-             v8::Local<v8::Object> obj = v8::Local<v8::Object>());
+  static FileHandle* New(Environment* env,
+                         int fd,
+                         v8::Local<v8::Object> obj = v8::Local<v8::Object>());
   virtual ~FileHandle();
 
   static void New(const v8::FunctionCallbackInfo<v8::Value>& args);
@@ -404,19 +410,19 @@ class FileHandle : public AsyncWrap, public StreamBase {
   FileHandle& operator=(const FileHandle&&) = delete;
 
  private:
+  FileHandle(Environment* env, v8::Local<v8::Object> obj, int fd);
+
   // Synchronous close that emits a warning
   void Close();
   void AfterClose();
 
   class CloseReq : public ReqWrap<uv_fs_t> {
    public:
     CloseReq(Environment* env,
+             Local<Object> obj,
              Local<Promise> promise,
              Local<Value> 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);
     }