Skip to content

Commit

Permalink
fs: fix potential segfault in async calls
Browse files Browse the repository at this point in the history 
When the async uv_fs_* call errors out synchronously in AsyncDestCall,
the after callbacks (e.g. AfterNoArgs) would delete the req_wrap
in FSReqAfterScope, and AsyncDestCall would set those req_wrap to
nullptr afterwards. But when it returns to the top-layer bindings,
the bindings all call `req_wrap->SetReturnValue()` again without
checking if `req_wrap` is nullptr, causing a segfault.

This has not been caught in any of the tests because we usually do a
lot of argument checking in the JS layer before invoking the uv_fs_*
functions, so it's rare to get a synchronous error from them.

Currently we never need the binding to return the wrap to JS layer,
so we can just call `req_wrap->SetReturnValue()` to return undefined
for normal FSReqWrap and the promise for FSReqPromise in AsyncDestCall
instead of doing this in the top-level bindings.

PR-URL: #18811
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: Tobias Nießen <tniessen@tnie.de>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
  • Loading branch information
@joyeecheung
joyeecheung committed Feb 19, 2018
1 parent 472cde6 commit 12412ef
Showing 1 changed file with 7 additions and 34 deletions.
41 changes: 7 additions & 34 deletions src/node_file.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -516,6 +516,7 @@ class fs_req_wrap {
DISALLOW_COPY_AND_ASSIGN(fs_req_wrap);
};

// Returns nullptr if the operation fails from the start.
template <typename Func, typename... Args>
inline FSReqBase* AsyncDestCall(Environment* env,
FSReqBase* req_wrap,
Expand All @@ -530,16 +531,16 @@ inline FSReqBase* AsyncDestCall(Environment* env,
uv_fs_t* uv_req = req_wrap->req();
uv_req->result = err;
uv_req->path = nullptr;
after(uv_req);
after(uv_req); // after may delete req_wrap if there is an error
req_wrap = nullptr;
} else {
req_wrap->SetReturnValue(args);
}

if (req_wrap != nullptr) {
args.GetReturnValue().Set(req_wrap->persistent());
}
return req_wrap;
}

// Returns nullptr if the operation fails from the start.
template <typename Func, typename... Args>
inline FSReqBase* AsyncCall(Environment* env,
FSReqBase* req_wrap,
Expand Down Expand Up @@ -618,7 +619,6 @@ void Access(const FunctionCallbackInfo<Value>& args) {
if (req_wrap != nullptr) { // access(path, mode, req)
AsyncCall(env, req_wrap, args, "access", UTF8, AfterNoArgs,
uv_fs_access, *path, mode);
req_wrap->SetReturnValue(args);
} else { // access(path, mode, undefined, ctx)
CHECK_EQ(argc, 4);
fs_req_wrap req_wrap;
Expand All @@ -640,7 +640,6 @@ void Close(const FunctionCallbackInfo<Value>& args) {
if (req_wrap != nullptr) { // close(fd, req)
AsyncCall(env, req_wrap, args, "close", UTF8, AfterNoArgs,
uv_fs_close, fd);
req_wrap->SetReturnValue(args);
} else { // close(fd, undefined, ctx)
CHECK_EQ(argc, 3);
fs_req_wrap req_wrap;
Expand Down Expand Up @@ -749,7 +748,6 @@ static void Stat(const FunctionCallbackInfo<Value>& args) {
if (req_wrap != nullptr) { // stat(path, req)
AsyncCall(env, req_wrap, args, "stat", UTF8, AfterStat,
uv_fs_stat, *path);
req_wrap->SetReturnValue(args);
} else { // stat(path, undefined, ctx)
CHECK_EQ(argc, 3);
fs_req_wrap req_wrap;
Expand All @@ -774,7 +772,6 @@ static void LStat(const FunctionCallbackInfo<Value>& args) {
if (req_wrap != nullptr) { // lstat(path, req)
AsyncCall(env, req_wrap, args, "lstat", UTF8, AfterStat,
uv_fs_lstat, *path);
req_wrap->SetReturnValue(args);
} else { // lstat(path, undefined, ctx)
CHECK_EQ(argc, 3);
fs_req_wrap req_wrap;
Expand All @@ -799,7 +796,6 @@ static void FStat(const FunctionCallbackInfo<Value>& args) {
if (req_wrap != nullptr) { // fstat(fd, req)
AsyncCall(env, req_wrap, args, "fstat", UTF8, AfterStat,
uv_fs_fstat, fd);
req_wrap->SetReturnValue(args);
} else { // fstat(fd, undefined, ctx)
CHECK_EQ(argc, 3);
fs_req_wrap req_wrap;
Expand Down Expand Up @@ -853,7 +849,6 @@ static void Link(const FunctionCallbackInfo<Value>& args) {
if (req_wrap != nullptr) { // link(src, dest, req)
AsyncDestCall(env, req_wrap, args, "link", *dest, dest.length(), UTF8,
AfterNoArgs, uv_fs_link, *src, *dest);
req_wrap->SetReturnValue(args);
} else { // link(src, dest)
CHECK_EQ(argc, 4);
fs_req_wrap req;
Expand All @@ -877,7 +872,6 @@ static void ReadLink(const FunctionCallbackInfo<Value>& args) {
if (req_wrap != nullptr) { // readlink(path, encoding, req)
AsyncCall(env, req_wrap, args, "readlink", encoding, AfterStringPtr,
uv_fs_readlink, *path);
req_wrap->SetReturnValue(args);
} else {
CHECK_EQ(argc, 4);
fs_req_wrap req;
Expand Down Expand Up @@ -918,7 +912,6 @@ static void Rename(const FunctionCallbackInfo<Value>& args) {
if (req_wrap != nullptr) {
AsyncDestCall(env, req_wrap, args, "rename", *new_path, new_path.length(),
UTF8, AfterNoArgs, uv_fs_rename, *old_path, *new_path);
req_wrap->SetReturnValue(args);
} else {
CHECK_EQ(argc, 4);
fs_req_wrap req;
Expand All @@ -942,7 +935,6 @@ static void FTruncate(const FunctionCallbackInfo<Value>& args) {
if (req_wrap != nullptr) {
AsyncCall(env, req_wrap, args, "ftruncate", UTF8, AfterNoArgs,
uv_fs_ftruncate, fd, len);
req_wrap->SetReturnValue(args);
} else {
CHECK_EQ(argc, 4);
fs_req_wrap req;
Expand All @@ -963,7 +955,6 @@ static void Fdatasync(const FunctionCallbackInfo<Value>& args) {
if (req_wrap != nullptr) {
AsyncCall(env, req_wrap, args, "fdatasync", UTF8, AfterNoArgs,
uv_fs_fdatasync, fd);
req_wrap->SetReturnValue(args);
} else {
CHECK_EQ(argc, 3);
fs_req_wrap req;
Expand All @@ -984,7 +975,6 @@ static void Fsync(const FunctionCallbackInfo<Value>& args) {
if (req_wrap != nullptr) {
AsyncCall(env, req_wrap, args, "fsync", UTF8, AfterNoArgs,
uv_fs_fsync, fd);
req_wrap->SetReturnValue(args);
} else {
CHECK_EQ(argc, 3);
fs_req_wrap req;
Expand All @@ -1005,7 +995,6 @@ static void Unlink(const FunctionCallbackInfo<Value>& args) {
if (req_wrap != nullptr) {
AsyncCall(env, req_wrap, args, "unlink", UTF8, AfterNoArgs,
uv_fs_unlink, *path);
req_wrap->SetReturnValue(args);
} else {
CHECK_EQ(argc, 3);
fs_req_wrap req;
Expand All @@ -1025,7 +1014,6 @@ static void RMDir(const FunctionCallbackInfo<Value>& args) {
if (req_wrap != nullptr) {
AsyncCall(env, req_wrap, args, "rmdir", UTF8, AfterNoArgs,
uv_fs_rmdir, *path);
req_wrap->SetReturnValue(args);
} else {
SYNC_CALL(rmdir, *path, *path)
}
Expand All @@ -1046,7 +1034,6 @@ static void MKDir(const FunctionCallbackInfo<Value>& args) {
if (req_wrap != nullptr) {
AsyncCall(env, req_wrap, args, "mkdir", UTF8, AfterNoArgs,
uv_fs_mkdir, *path, mode);
req_wrap->SetReturnValue(args);
} else {
SYNC_CALL(mkdir, *path, *path, mode)
}
Expand All @@ -1064,7 +1051,6 @@ static void RealPath(const FunctionCallbackInfo<Value>& args) {
if (req_wrap != nullptr) {
AsyncCall(env, req_wrap, args, "realpath", encoding, AfterStringPtr,
uv_fs_realpath, *path);
req_wrap->SetReturnValue(args);
} else {
SYNC_CALL(realpath, *path, *path);
const char* link_path = static_cast<const char*>(SYNC_REQ.ptr);
Expand Down Expand Up @@ -1096,7 +1082,6 @@ static void ReadDir(const FunctionCallbackInfo<Value>& args) {
if (req_wrap != nullptr) {
AsyncCall(env, req_wrap, args, "scandir", encoding, AfterScanDir,
uv_fs_scandir, *path, 0 /*flags*/);
req_wrap->SetReturnValue(args);
} else {
SYNC_CALL(scandir, *path, *path, 0 /*flags*/)

Expand Down Expand Up @@ -1167,7 +1152,6 @@ static void Open(const FunctionCallbackInfo<Value>& args) {
if (req_wrap != nullptr) {
AsyncCall(env, req_wrap, args, "open", UTF8, AfterInteger,
uv_fs_open, *path, flags, mode);
req_wrap->SetReturnValue(args);
} else {
SYNC_CALL(open, *path, *path, flags, mode)
args.GetReturnValue().Set(SYNC_RESULT);
Expand All @@ -1192,7 +1176,6 @@ static void OpenFileHandle(const FunctionCallbackInfo<Value>& args) {
if (req_wrap != nullptr) {
AsyncCall(env, req_wrap, args, "open", UTF8, AfterOpenFileHandle,
uv_fs_open, *path, flags, mode);
req_wrap->SetReturnValue(args);
} else {
SYNC_CALL(open, *path, *path, flags, mode)
HandleScope scope(env->isolate());
Expand All @@ -1217,7 +1200,6 @@ static void CopyFile(const FunctionCallbackInfo<Value>& args) {
if (req_wrap != nullptr) {
AsyncCall(env, req_wrap, args, "copyfile", UTF8, AfterNoArgs,
uv_fs_copyfile, *src, *dest, flags);
req_wrap->SetReturnValue(args);
} else {
SYNC_DEST_CALL(copyfile, *src, *dest, *src, *dest, flags)
}
Expand Down Expand Up @@ -1260,7 +1242,7 @@ static void WriteBuffer(const FunctionCallbackInfo<Value>& args) {
if (req_wrap != nullptr) {
AsyncCall(env, req_wrap, args, "write", UTF8, AfterInteger,
uv_fs_write, fd, &uvbuf, 1, pos);
return req_wrap->SetReturnValue(args);
return;
}

SYNC_CALL(write, nullptr, fd, &uvbuf, 1, pos)
Expand Down Expand Up @@ -1297,7 +1279,7 @@ static void WriteBuffers(const FunctionCallbackInfo<Value>& args) {
if (req_wrap != nullptr) {
AsyncCall(env, req_wrap, args, "write", UTF8, AfterInteger,
uv_fs_write, fd, *iovs, iovs.length(), pos);
return req_wrap->SetReturnValue(args);
return;
}

SYNC_CALL(write, nullptr, fd, *iovs, iovs.length(), pos)
Expand Down Expand Up @@ -1365,7 +1347,6 @@ static void WriteString(const FunctionCallbackInfo<Value>& args) {
if (req_wrap != nullptr) {
AsyncCall(env, req_wrap, args, "write", UTF8, AfterInteger,
uv_fs_write, fd, &uvbuf, 1, pos);
req_wrap->SetReturnValue(args);
} else {
SYNC_CALL(write, nullptr, fd, &uvbuf, 1, pos)
return args.GetReturnValue().Set(SYNC_RESULT);
Expand Down Expand Up @@ -1420,7 +1401,6 @@ static void Read(const FunctionCallbackInfo<Value>& args) {
if (req_wrap != nullptr) {
AsyncCall(env, req_wrap, args, "read", UTF8, AfterInteger,
uv_fs_read, fd, &uvbuf, 1, pos);
req_wrap->SetReturnValue(args);
} else {
SYNC_CALL(read, 0, fd, &uvbuf, 1, pos)
args.GetReturnValue().Set(SYNC_RESULT);
Expand All @@ -1446,7 +1426,6 @@ static void Chmod(const FunctionCallbackInfo<Value>& args) {
if (req_wrap != nullptr) {
AsyncCall(env, req_wrap, args, "chmod", UTF8, AfterNoArgs,
uv_fs_chmod, *path, mode);
req_wrap->SetReturnValue(args);
} else {
SYNC_CALL(chmod, *path, *path, mode);
}
Expand All @@ -1469,7 +1448,6 @@ static void FChmod(const FunctionCallbackInfo<Value>& args) {
if (req_wrap != nullptr) {
AsyncCall(env, req_wrap, args, "fchmod", UTF8, AfterNoArgs,
uv_fs_fchmod, fd, mode);
req_wrap->SetReturnValue(args);
} else {
SYNC_CALL(fchmod, 0, fd, mode);
}
Expand Down Expand Up @@ -1497,7 +1475,6 @@ static void Chown(const FunctionCallbackInfo<Value>& args) {
if (req_wrap != nullptr) {
AsyncCall(env, req_wrap, args, "chown", UTF8, AfterNoArgs,
uv_fs_chown, *path, uid, gid);
req_wrap->SetReturnValue(args);
} else {
SYNC_CALL(chown, *path, *path, uid, gid);
}
Expand All @@ -1522,7 +1499,6 @@ static void FChown(const FunctionCallbackInfo<Value>& args) {
if (req_wrap != nullptr) {
AsyncCall(env, req_wrap, args, "fchown", UTF8, AfterNoArgs,
uv_fs_fchown, fd, uid, gid);
req_wrap->SetReturnValue(args);
} else {
SYNC_CALL(fchown, 0, fd, uid, gid);
}
Expand All @@ -1546,7 +1522,6 @@ static void UTimes(const FunctionCallbackInfo<Value>& args) {
if (req_wrap != nullptr) {
AsyncCall(env, req_wrap, args, "utime", UTF8, AfterNoArgs,
uv_fs_utime, *path, atime, mtime);
req_wrap->SetReturnValue(args);
} else {
SYNC_CALL(utime, *path, *path, atime, mtime);
}
Expand All @@ -1567,7 +1542,6 @@ static void FUTimes(const FunctionCallbackInfo<Value>& args) {
if (req_wrap != nullptr) {
AsyncCall(env, req_wrap, args, "futime", UTF8, AfterNoArgs,
uv_fs_futime, fd, atime, mtime);
req_wrap->SetReturnValue(args);
} else {
SYNC_CALL(futime, 0, fd, atime, mtime);
}
Expand All @@ -1587,7 +1561,6 @@ static void Mkdtemp(const FunctionCallbackInfo<Value>& args) {
if (req_wrap != nullptr) {
AsyncCall(env, req_wrap, args, "mkdtemp", encoding, AfterStringPath,
uv_fs_mkdtemp, *tmpl);
req_wrap->SetReturnValue(args);
} else {
SYNC_CALL(mkdtemp, *tmpl, *tmpl);
const char* path = static_cast<const char*>(SYNC_REQ.path);
Expand Down

0 comments on commit 12412ef

Please sign in to comment.