Skip to content
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.

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

fs: fix potential segfault in async calls #18811

Closed
wants to merge 2 commits into from

Conversation

joyeecheung
Copy link
Member

@joyeecheung joyeecheung commented Feb 16, 2018

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.

Checklist
  • make -j4 test (UNIX), or vcbuild test (Windows) passes
  • tests and/or benchmarks are included
  • documentation is changed or added
  • commit message follows commit guidelines
Affected core subsystem(s)

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.
@nodejs-github-bot nodejs-github-bot added c++ Issues and PRs that require attention from people who are familiar with C++. fs Issues and PRs related to the fs subsystem / file system. labels Feb 16, 2018
@joyeecheung
Copy link
Member Author

The test case that I found that could trigger this:

fs.copyFile(file1, file2, -1, (err) => { console.log(err) })

Before this patch, it segfaults, after this patch, the loop is still kept open after the callback being called with a correct error (EINVAL). There might be something wrong with uv_fs_copyfile or the way we clean up uv_fs_t*s. cc @cjihrig

Copy link
Member

@bnoordhuis bnoordhuis left a comment

Choose a reason for hiding this comment

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

LGTM and good catch.

src/node_file.cc Outdated
@@ -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

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.

src/node_file.cc Outdated
req_wrap = nullptr;
}

if (req_wrap != nullptr) {
args.GetReturnValue().Set(req_wrap->persistent());
req_wrap->SetReturnValue(args);
Copy link
Member Author

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

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

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

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

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

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

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.

@joyeecheung
Copy link
Member Author

Upstream PR opened in libuv/libuv#1747 . I can open a PR later to add a complete test for copyFile, or add that in one of my synchronous error migration PRs (this bug was found during the migration anyway)

Copy link
Member

@addaleax addaleax left a comment

Choose a reason for hiding this comment

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

Thanks for this!

@joyeecheung
Copy link
Member Author

Copy link
Member

@jasnell jasnell left a comment

Choose a reason for hiding this comment

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

Can this include a test?

@joyeecheung
Copy link
Member Author

@jasnell I think the only reliable errors that can be triggered this way via public APIs are the EINVAL from uv_fs_copyfile, others are either caused by malloc (flaky to test) or invalid buffers (guarded against in bindings). As described in #18811 (comment), there is a bug in uv_fs_copyfile that keeps the loop open if EINVAL is returned, so we cannot reliably test this until that gets fixed upstream.

@joyeecheung
Copy link
Member Author

@joyeecheung joyeecheung added the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Feb 18, 2018
@joyeecheung
Copy link
Member Author

CI failures are unrelated. Landed in 12412ef. I will add a test when the upstream is fixed.

joyeecheung added a commit that referenced this pull request Feb 19, 2018
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 <[email protected]>
Reviewed-By: Tobias Nießen <[email protected]>
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
cjihrig added a commit to cjihrig/libuv that referenced this pull request Feb 21, 2018
On Unix, if a fs function fails validation after INIT but
before sending the work to the thread pool, then is is
necessary to manually unregister the request. This commit
moves the registration to just before the work submission.
This also makes Unix match the Windows behavior.

Refs: libuv#1747
Refs: nodejs/node#18811
PR-URL: libuv#1751
Reviewed-By: Ben Noordhuis <[email protected]>
cjihrig pushed a commit to cjihrig/libuv that referenced this pull request Feb 21, 2018
This commit adds tests that pass bad options to uv_fs_copyfile(),
uv_fs_read(), and uv_fs_write(). These tests verify that the
asynchronous version of these functions do not hold the event
loop open on bad inputs.

Refs: nodejs/node#18811
PR-URL: libuv#1747
Reviewed-By: Colin Ihrig <[email protected]>
@MylesBorins
Copy link
Contributor

Should this be backported to v9.x-staging? If yes please follow the guide and raise a backport PR, if not let me know or add the dont-land-on label.

@joyeecheung
Copy link
Member Author

This bug came from the fs promise API so does not make sense to backport if there is no plan to backport that PR. cc @jasnell Added don't land labels for now.

@joyeecheung joyeecheung added dont-land-on-v8.x and removed backport-requested-v9.x author ready PRs that have at least one approval, no pending requests for changes, and a CI started. labels Feb 21, 2018
MayaLekova pushed a commit to MayaLekova/node that referenced this pull request May 8, 2018
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: nodejs#18811
Reviewed-By: Ben Noordhuis <[email protected]>
Reviewed-By: Tobias Nießen <[email protected]>
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
c++ Issues and PRs that require attention from people who are familiar with C++. fs Issues and PRs related to the fs subsystem / file system.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants