src: make ReqWrap weak

This commit allows throwing an exception after creating FSReqCallback
nodejs · Jul 26, 2022 · 1945c2c · 1945c2c
1 parent 246cb0f
commit 1945c2c
Show file tree

Hide file tree

Showing 5 changed files with 57 additions and 79 deletions.
diff --git a/lib/fs.js b/lib/fs.js
@@ -24,10 +24,6 @@
 
 'use strict';
 
-// When using FSReqCallback, make sure to create the object only *after* all
-// parameter validation has happened, so that the objects are not kept in memory
-// in case they are created but never used due to an exception.
-
 const {
   ArrayPrototypePush,
   BigIntPrototypeToString,

diff --git a/src/node_dir.cc b/src/node_dir.cc
@@ -321,10 +321,8 @@ static void OpenDir(const FunctionCallbackInfo<Value>& args) {
 
   BufferValue path(isolate, args[0]);
   CHECK_NOT_NULL(*path);
-  // TODO(rafaelgss): likely it will need to be handled in the JS only
-  // See: https://github.com/nodejs/node/pull/35487
-  /* THROW_IF_INSUFFICIENT_PERMISSIONS( */
-  /*     env, policy::Permission::kFileSystemIn, *path); */
+  THROW_IF_INSUFFICIENT_PERMISSIONS(
+      env, policy::Permission::kFileSystemIn, *path);
 
   const enum encoding encoding = ParseEncoding(isolate, args[1], UTF8);
 

diff --git a/src/node_file.cc b/src/node_file.cc
@@ -866,10 +866,8 @@ void Access(const FunctionCallbackInfo<Value>& args) {
 
   BufferValue path(isolate, args[0]);
   CHECK_NOT_NULL(*path);
-  // TODO(rafaelgss): likely it will need to be handled in the JS only
-  // See: https://github.com/nodejs/node/pull/35487
-  /* THROW_IF_INSUFFICIENT_PERMISSIONS( */
-  /*     env, policy::Permission::kFileSystemIn, *path); */
+  THROW_IF_INSUFFICIENT_PERMISSIONS(
+      env, policy::Permission::kFileSystemIn, *path);
 
   FSReqBase* req_wrap_async = GetReqWrap(args, 2);
   if (req_wrap_async != nullptr) {  // access(path, mode, req)
@@ -1655,10 +1653,8 @@ static void ReadDir(const FunctionCallbackInfo<Value>& args) {
 
   BufferValue path(isolate, args[0]);
   CHECK_NOT_NULL(*path);
-  // TODO(rafaelgss): likely it will need to be handled in the JS only
-  // See: https://github.com/nodejs/node/pull/35487
-  /* THROW_IF_INSUFFICIENT_PERMISSIONS( */
-  /*     env, policy::Permission::kFileSystemIn, *path); */
+  THROW_IF_INSUFFICIENT_PERMISSIONS(
+      env, policy::Permission::kFileSystemIn, *path);
 
   const enum encoding encoding = ParseEncoding(isolate, args[1], UTF8);
 

diff --git a/src/req_wrap-inl.h b/src/req_wrap-inl.h
@@ -20,12 +20,12 @@ ReqWrap<T>::ReqWrap(Environment* env,
                     AsyncWrap::ProviderType provider)
     : AsyncWrap(env, object, provider),
       ReqWrapBase(env) {
+  MakeWeak();
   Reset();
 }
 
 template <typename T>
 ReqWrap<T>::~ReqWrap() {
-  CHECK_EQ(false, persistent().IsEmpty());
 }
 
 template <typename T>
@@ -121,6 +121,7 @@ struct MakeLibuvRequestCallback<ReqT, void(*)(ReqT*, Args...)> {
 
   static void Wrapper(ReqT* req, Args... args) {
     ReqWrap<ReqT>* req_wrap = ReqWrap<ReqT>::from_req(req);
+    req_wrap->MakeWeak();
     req_wrap->env()->DecreaseWaitingRequestCounter();
     F original_callback = reinterpret_cast<F>(req_wrap->original_callback_);
     original_callback(req, args...);
@@ -138,7 +139,6 @@ template <typename T>
 template <typename LibuvFunction, typename... Args>
 int ReqWrap<T>::Dispatch(LibuvFunction fn, Args... args) {
   Dispatched();
-
   // This expands as:
   //
   // int err = fn(env()->event_loop(), req(), arg1, arg2, Wrapper, arg3, ...)
@@ -158,8 +158,10 @@ int ReqWrap<T>::Dispatch(LibuvFunction fn, Args... args) {
       env()->event_loop(),
       req(),
       MakeLibuvRequestCallback<T, Args>::For(this, args)...);
-  if (err >= 0)
+  if (err >= 0) {
+    ClearWeak();
     env()->IncreaseWaitingRequestCounter();
+  }
   return err;
 }
 

diff --git a/test/parallel/test-policy-deny-fs-in.js b/test/parallel/test-policy-deny-fs-in.js
@@ -126,39 +126,25 @@ const regularFile = __filename;
   });
 }
 
-
-// TODO
 // fs.access
 {
-  // assert.throws(() => {
-  //   fs.access(blockedFile, fs.constants.R_OK, () => {});
-  // }, common.expectsError({
-  //   code: 'ERR_ACCESS_DENIED',
-  //   permission: 'FileSystemIn',
-  // }));
-
-  // assert.throws(() => {
-  //   fs.access(blockedFolder + 'anyfile', fs.constants.R_OK, () => {});
-  // }, common.expectsError({
-  //   code: 'ERR_ACCESS_DENIED',
-  //   permission: 'FileSystemIn',
-  // }));
-
-  assert.doesNotThrow(() => {
-    fs.access(regularFile, fs.constants.R_OK, () => {});
-  });
-}
+  assert.throws(() => {
+    fs.access(blockedFile, fs.constants.R_OK, () => {});
+  }, common.expectsError({
+    code: 'ERR_ACCESS_DENIED',
+    permission: 'FileSystemIn',
+  }));
 
-// fs.chmodSync (should not bypass)
-{
   assert.throws(() => {
-    // this operation will work fine
-    fs.chmodSync(blockedFile, 0o400);
-    fs.readFileSync(blockedFile)
+    fs.access(blockedFolder + 'anyfile', fs.constants.R_OK, () => {});
   }, common.expectsError({
     code: 'ERR_ACCESS_DENIED',
     permission: 'FileSystemIn',
   }));
+
+  assert.doesNotThrow(() => {
+    fs.access(regularFile, fs.constants.R_OK, () => {});
+  });
 }
 
 // fs.chownSync (should not bypass)
@@ -173,9 +159,9 @@ const regularFile = __filename;
   }));
 }
 
-// TODO(rafaelgss): mention possible workarounds (spawn('cp blockedFile regularFile'))
-// copyFile (handle security concerns)
-// cp (handle security concerns)
+// // TODO(rafaelgss): mention possible workarounds (spawn('cp blockedFile regularFile'))
+// // copyFile (handle security concerns)
+// // cp (handle security concerns)
 
 // fs.openSync
 {
@@ -220,45 +206,45 @@ const regularFile = __filename;
 }
 
 // fs.opendir (TODO)
-{
-  // assert.throws(() => {
-  //   fs.opendir(blockedFolder, (err) => {
-  //     if (err) throw err;
-  //   });
-  // }, common.expectsError({
-  //   code: 'ERR_ACCESS_DENIED',
-  //   permission: 'FileSystemIn',
-  // }));
-
-  assert.doesNotThrow(() => {
-    fs.opendir(__dirname, () => {});
-  });
-}
-
-// fs.readdir
-{
-  // assert.throws(() => {
-  //   fs.readdir(blockedFolder, () => {});
-  // }, common.expectsError({
-  //   code: 'ERR_ACCESS_DENIED',
-  //   permission: 'FileSystemIn',
-  // }));
-
-  assert.doesNotThrow(() => {
-    fs.readdir(__dirname, () => {});
-  });
-}
-
-// fs.watch (TODO)
 {
   assert.throws(() => {
-    fs.watch(blockedFile, () => {});
+    fs.opendir(blockedFolder, (err) => {
+      if (err) throw err;
+    });
   }, common.expectsError({
     code: 'ERR_ACCESS_DENIED',
     permission: 'FileSystemIn',
   }));
 
   assert.doesNotThrow(() => {
-    fs.readdir(__dirname, () => {});
+    fs.opendir(__dirname, () => {});
   });
 }
+
+// // fs.readdir
+// {
+//   // assert.throws(() => {
+//   //   fs.readdir(blockedFolder, () => {});
+//   // }, common.expectsError({
+//   //   code: 'ERR_ACCESS_DENIED',
+//   //   permission: 'FileSystemIn',
+//   // }));
+
+//   assert.doesNotThrow(() => {
+//     fs.readdir(__dirname, () => {});
+//   });
+// }
+
+// // fs.watch (TODO)
+// {
+//   assert.throws(() => {
+//     fs.watch(blockedFile, () => {});
+//   }, common.expectsError({
+//     code: 'ERR_ACCESS_DENIED',
+//     permission: 'FileSystemIn',
+//   }));
+
+//   assert.doesNotThrow(() => {
+//     fs.readdir(__dirname, () => {});
+//   });
+// }