Skip to content

fs: fix potential segfault in async calls #18811

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
wants to merge 2 commits into from
Closed

fs: fix potential segfault in async calls #18811

Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
1fe23e0
fs: fix potential segfault in async calls
joyeecheung Feb 16, 2018
224e88c
squash! merge conditionals in AsyncCall
joyeecheung Feb 17, 2018
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
38 changes: 6 additions & 32 deletions src/node_file.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -518,6 +518,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 @@ -532,16 +533,17 @@ 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;
}

if (req_wrap != nullptr) {
Copy link
Member

@bnoordhuis bnoordhuis Feb 16, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Since you're here, maybe change this if to an else of the previous conditional. Makes the logic a little more obvious, IMO.

args.GetReturnValue().Set(req_wrap->persistent());
req_wrap->SetReturnValue(args);
Copy link
Member Author

@joyeecheung joyeecheung Feb 16, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Question: it is possible for after being called in another thread in the middle of this and make req_wrap a dangling pointer here? In my experiments if I move the check outside to the bindings, it seems possible.. Maybe I could set uv_req->data = nullptr in FSReqAfterScope (or ~ReqWrap even?) and check uv_req->data instead to be safe... cc @bnoordhuis

Copy link
Member

@bnoordhuis bnoordhuis Feb 16, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The after callback should always run on the main thread; only the work callback runs in a different thread.

Copy link
Member Author

@joyeecheung joyeecheung Feb 16, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@bnoordhuis Just to make sure I am understanding correctly here..can I assume the uv_fs_cb passed to uv_fs_* will not get called if uv_fs_* returns an error right away?

Copy link
Member

@bnoordhuis bnoordhuis Feb 16, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That's right.

Copy link
Member Author

@joyeecheung joyeecheung Feb 16, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@bnoordhuis Thanks, I think I understand what's going on with uv_fs_copyfile keeping the loop open when it errors out with EINVAL now..this seems to fix the problem:

diff --git a/deps/uv/src/unix/fs.c b/deps/uv/src/unix/fs.c
index e0969a4c2f..f0446110f2 100644
--- a/deps/uv/src/unix/fs.c
+++ b/deps/uv/src/unix/fs.c
@@ -1513,8 +1513,11 @@ int uv_fs_copyfile(uv_loop_t* loop,
                    uv_fs_cb cb) {
   INIT(COPYFILE);

-  if (flags & ~UV_FS_COPYFILE_EXCL)
+  if (flags & ~UV_FS_COPYFILE_EXCL) {
+    if (cb != NULL)
+      uv__req_unregister(loop, req);
     return -EINVAL;
+  }

   PATH2;
   req->flags = flags;

Copy link
Member

@bnoordhuis bnoordhuis Feb 16, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah, the bug is obvious in hindsight... the flags check should be done before INIT(COPYFILE). Do you want to open a PR or should I do it?

Copy link
Member Author

@joyeecheung joyeecheung Feb 16, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@bnoordhuis Yes, checking the flags before INIT seems more reasonable...would love to try to open a PR to libuv since I have not done that before. I will do that once I get to somewhere I can open my laptop.

}
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 @@ -620,7 +622,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 @@ -642,7 +643,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 @@ -751,7 +751,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 @@ -776,7 +775,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 @@ -801,7 +799,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 @@ -855,7 +852,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 @@ -879,7 +875,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 @@ -920,7 +915,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 @@ -944,7 +938,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 @@ -965,7 +958,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 @@ -986,7 +978,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 @@ -1007,7 +998,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 @@ -1027,7 +1017,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 @@ -1048,7 +1037,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 @@ -1066,7 +1054,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 @@ -1098,7 +1085,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 @@ -1169,7 +1155,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 @@ -1194,7 +1179,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 @@ -1219,7 +1203,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 @@ -1262,7 +1245,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 @@ -1299,7 +1282,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 @@ -1367,7 +1350,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 @@ -1422,7 +1404,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 @@ -1448,7 +1429,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 @@ -1471,7 +1451,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 @@ -1499,7 +1478,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 @@ -1524,7 +1502,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 @@ -1548,7 +1525,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 @@ -1569,7 +1545,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 @@ -1589,7 +1564,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