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
joyeecheung 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
Prev Previous commit
squash! merge conditionals in AsyncCall
  • Loading branch information
@joyeecheung
joyeecheung committed Feb 17, 2018
commit 224e88c4cf01d489ef75e39a2500d9e23c954e6f
5 changes: 2 additions & 3 deletions src/node_file.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -535,11 +535,10 @@ inline FSReqBase* AsyncDestCall(Environment* env,
uv_req->path = nullptr;
after(uv_req); // after may delete req_wrap if there is an error
req_wrap = nullptr;
}

if (req_wrap != nullptr) {
} else {
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;
}

Expand Down