Skip to content

Commit 1945c2c

Browse files
RafaelGSSRafaelGSS
committed
src: make ReqWrap weak
This commit allows throwing an exception after creating FSReqCallback
1 parent 246cb0f commit 1945c2c

File tree

5 files changed

+57
-79
lines changed

5 files changed

+57
-79
lines changed

‎lib/fs.js‎

Lines changed: 0 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -24,10 +24,6 @@
2424

2525
'use strict';
2626

27-
// When using FSReqCallback, make sure to create the object only *after* all
28-
// parameter validation has happened, so that the objects are not kept in memory
29-
// in case they are created but never used due to an exception.
30-
3127
const {
3228
ArrayPrototypePush,
3329
BigIntPrototypeToString,

‎src/node_dir.cc‎

Lines changed: 2 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -321,10 +321,8 @@ static void OpenDir(const FunctionCallbackInfo<Value>& args) {
321321

322322
BufferValue path(isolate, args[0]);
323323
CHECK_NOT_NULL(*path);
324-
// TODO(rafaelgss): likely it will need to be handled in the JS only
325-
// See: https://github.com/nodejs/node/pull/35487
326-
/* THROW_IF_INSUFFICIENT_PERMISSIONS( */
327-
/* env, policy::Permission::kFileSystemIn, *path); */
324+
THROW_IF_INSUFFICIENT_PERMISSIONS(
325+
env, policy::Permission::kFileSystemIn, *path);
328326

329327
const enum encoding encoding = ParseEncoding(isolate, args[1], UTF8);
330328

‎src/node_file.cc‎

Lines changed: 4 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -866,10 +866,8 @@ void Access(const FunctionCallbackInfo<Value>& args) {
866866

867867
BufferValue path(isolate, args[0]);
868868
CHECK_NOT_NULL(*path);
869-
// TODO(rafaelgss): likely it will need to be handled in the JS only
870-
// See: https://github.com/nodejs/node/pull/35487
871-
/* THROW_IF_INSUFFICIENT_PERMISSIONS( */
872-
/* env, policy::Permission::kFileSystemIn, *path); */
869+
THROW_IF_INSUFFICIENT_PERMISSIONS(
870+
env, policy::Permission::kFileSystemIn, *path);
873871

874872
FSReqBase* req_wrap_async = GetReqWrap(args, 2);
875873
if (req_wrap_async != nullptr) { // access(path, mode, req)
@@ -1655,10 +1653,8 @@ static void ReadDir(const FunctionCallbackInfo<Value>& args) {
16551653

16561654
BufferValue path(isolate, args[0]);
16571655
CHECK_NOT_NULL(*path);
1658-
// TODO(rafaelgss): likely it will need to be handled in the JS only
1659-
// See: https://github.com/nodejs/node/pull/35487
1660-
/* THROW_IF_INSUFFICIENT_PERMISSIONS( */
1661-
/* env, policy::Permission::kFileSystemIn, *path); */
1656+
THROW_IF_INSUFFICIENT_PERMISSIONS(
1657+
env, policy::Permission::kFileSystemIn, *path);
16621658

16631659
const enum encoding encoding = ParseEncoding(isolate, args[1], UTF8);
16641660

‎src/req_wrap-inl.h‎

Lines changed: 5 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -20,12 +20,12 @@ ReqWrap<T>::ReqWrap(Environment* env,
2020
AsyncWrap::ProviderType provider)
2121
: AsyncWrap(env, object, provider),
2222
ReqWrapBase(env) {
23+
MakeWeak();
2324
Reset();
2425
}
2526

2627
template <typename T>
2728
ReqWrap<T>::~ReqWrap() {
28-
CHECK_EQ(false, persistent().IsEmpty());
2929
}
3030

3131
template <typename T>
@@ -121,6 +121,7 @@ struct MakeLibuvRequestCallback<ReqT, void(*)(ReqT*, Args...)> {
121121

122122
static void Wrapper(ReqT* req, Args... args) {
123123
ReqWrap<ReqT>* req_wrap = ReqWrap<ReqT>::from_req(req);
124+
req_wrap->MakeWeak();
124125
req_wrap->env()->DecreaseWaitingRequestCounter();
125126
F original_callback = reinterpret_cast<F>(req_wrap->original_callback_);
126127
original_callback(req, args...);
@@ -138,7 +139,6 @@ template <typename T>
138139
template <typename LibuvFunction, typename... Args>
139140
int ReqWrap<T>::Dispatch(LibuvFunction fn, Args... args) {
140141
Dispatched();
141-
142142
// This expands as:
143143
//
144144
// int err = fn(env()->event_loop(), req(), arg1, arg2, Wrapper, arg3, ...)
@@ -158,8 +158,10 @@ int ReqWrap<T>::Dispatch(LibuvFunction fn, Args... args) {
158158
env()->event_loop(),
159159
req(),
160160
MakeLibuvRequestCallback<T, Args>::For(this, args)...);
161-
if (err >= 0)
161+
if (err >= 0) {
162+
ClearWeak();
162163
env()->IncreaseWaitingRequestCounter();
164+
}
163165
return err;
164166
}
165167

‎test/parallel/test-policy-deny-fs-in.js‎

Lines changed: 46 additions & 60 deletions
Original file line numberDiff line numberDiff line change
@@ -126,39 +126,25 @@ const regularFile = __filename;
126126
});
127127
}
128128

129-
130-
// TODO
131129
// fs.access
132130
{
133-
// assert.throws(() => {
134-
// fs.access(blockedFile, fs.constants.R_OK, () => {});
135-
// }, common.expectsError({
136-
// code: 'ERR_ACCESS_DENIED',
137-
// permission: 'FileSystemIn',
138-
// }));
139-
140-
// assert.throws(() => {
141-
// fs.access(blockedFolder + 'anyfile', fs.constants.R_OK, () => {});
142-
// }, common.expectsError({
143-
// code: 'ERR_ACCESS_DENIED',
144-
// permission: 'FileSystemIn',
145-
// }));
146-
147-
assert.doesNotThrow(() => {
148-
fs.access(regularFile, fs.constants.R_OK, () => {});
149-
});
150-
}
131+
assert.throws(() => {
132+
fs.access(blockedFile, fs.constants.R_OK, () => {});
133+
}, common.expectsError({
134+
code: 'ERR_ACCESS_DENIED',
135+
permission: 'FileSystemIn',
136+
}));
151137

152-
// fs.chmodSync (should not bypass)
153-
{
154138
assert.throws(() => {
155-
// this operation will work fine
156-
fs.chmodSync(blockedFile, 0o400);
157-
fs.readFileSync(blockedFile)
139+
fs.access(blockedFolder + 'anyfile', fs.constants.R_OK, () => {});
158140
}, common.expectsError({
159141
code: 'ERR_ACCESS_DENIED',
160142
permission: 'FileSystemIn',
161143
}));
144+
145+
assert.doesNotThrow(() => {
146+
fs.access(regularFile, fs.constants.R_OK, () => {});
147+
});
162148
}
163149

164150
// fs.chownSync (should not bypass)
@@ -173,9 +159,9 @@ const regularFile = __filename;
173159
}));
174160
}
175161

176-
// TODO(rafaelgss): mention possible workarounds (spawn('cp blockedFile regularFile'))
177-
// copyFile (handle security concerns)
178-
// cp (handle security concerns)
162+
// // TODO(rafaelgss): mention possible workarounds (spawn('cp blockedFile regularFile'))
163+
// // copyFile (handle security concerns)
164+
// // cp (handle security concerns)
179165

180166
// fs.openSync
181167
{
@@ -220,45 +206,45 @@ const regularFile = __filename;
220206
}
221207

222208
// fs.opendir (TODO)
223-
{
224-
// assert.throws(() => {
225-
// fs.opendir(blockedFolder, (err) => {
226-
// if (err) throw err;
227-
// });
228-
// }, common.expectsError({
229-
// code: 'ERR_ACCESS_DENIED',
230-
// permission: 'FileSystemIn',
231-
// }));
232-
233-
assert.doesNotThrow(() => {
234-
fs.opendir(__dirname, () => {});
235-
});
236-
}
237-
238-
// fs.readdir
239-
{
240-
// assert.throws(() => {
241-
// fs.readdir(blockedFolder, () => {});
242-
// }, common.expectsError({
243-
// code: 'ERR_ACCESS_DENIED',
244-
// permission: 'FileSystemIn',
245-
// }));
246-
247-
assert.doesNotThrow(() => {
248-
fs.readdir(__dirname, () => {});
249-
});
250-
}
251-
252-
// fs.watch (TODO)
253209
{
254210
assert.throws(() => {
255-
fs.watch(blockedFile, () => {});
211+
fs.opendir(blockedFolder, (err) => {
212+
if (err) throw err;
213+
});
256214
}, common.expectsError({
257215
code: 'ERR_ACCESS_DENIED',
258216
permission: 'FileSystemIn',
259217
}));
260218

261219
assert.doesNotThrow(() => {
262-
fs.readdir(__dirname, () => {});
220+
fs.opendir(__dirname, () => {});
263221
});
264222
}
223+
224+
// // fs.readdir
225+
// {
226+
// // assert.throws(() => {
227+
// // fs.readdir(blockedFolder, () => {});
228+
// // }, common.expectsError({
229+
// // code: 'ERR_ACCESS_DENIED',
230+
// // permission: 'FileSystemIn',
231+
// // }));
232+
233+
// assert.doesNotThrow(() => {
234+
// fs.readdir(__dirname, () => {});
235+
// });
236+
// }
237+
238+
// // fs.watch (TODO)
239+
// {
240+
// assert.throws(() => {
241+
// fs.watch(blockedFile, () => {});
242+
// }, common.expectsError({
243+
// code: 'ERR_ACCESS_DENIED',
244+
// permission: 'FileSystemIn',
245+
// }));
246+
247+
// assert.doesNotThrow(() => {
248+
// fs.readdir(__dirname, () => {});
249+
// });
250+
// }

0 commit comments

Comments
 (0)