fs: fix potential segfault in async calls #18811

joyeecheung · 2018-02-16T09:03:11Z

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.

joyeecheung · 2018-02-16T09:12:15Z

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

bnoordhuis

LGTM and good catch.

bnoordhuis · 2018-02-16T09:12:27Z

src/node_file.cc

@@ -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) {


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

joyeecheung · 2018-02-16T09:18:50Z

src/node_file.cc

    req_wrap = nullptr;
  }

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


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

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

@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?

That's right.

@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;

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?

@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 · 2018-02-16T13:00:32Z

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)

addaleax

Thanks for this!

joyeecheung · 2018-02-16T13:19:07Z

CI: https://ci.nodejs.org/job/node-test-pull-request/13197/

jasnell

Can this include a test?

joyeecheung · 2018-02-16T14:46:04Z

@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 · 2018-02-17T08:41:06Z

Addressed #18811 (comment). New CI: https://ci.nodejs.org/job/node-test-pull-request/13238/

joyeecheung · 2018-02-19T07:13:36Z

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

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>

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 <info@bnoordhuis.nl>

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 <cjihrig@gmail.com>

MylesBorins · 2018-02-21T15:12:33Z

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 · 2018-02-21T18:21:45Z

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.

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 <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>

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

bnoordhuis approved these changes Feb 16, 2018

View reviewed changes

joyeecheung commented Feb 16, 2018

View reviewed changes

joyeecheung requested a review from jasnell February 16, 2018 09:22

tniessen approved these changes Feb 16, 2018

View reviewed changes

joyeecheung mentioned this pull request Feb 16, 2018

src: unregister requests on invalid input libuv/libuv#1747

Closed

addaleax approved these changes Feb 16, 2018

View reviewed changes

joyeecheung mentioned this pull request Feb 16, 2018

src: fix the returned Promise of fs.promises.symlink #18791

Closed

2 tasks

jasnell approved these changes Feb 16, 2018

View reviewed changes

cjihrig approved these changes Feb 16, 2018

View reviewed changes

squash! merge conditionals in AsyncCall

224e88c

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 closed this Feb 19, 2018

joyeecheung mentioned this pull request Feb 20, 2018

fs.promises.symlink() does not return the right Promise #18674

Closed

MylesBorins added the backport-requested-v9.x label Feb 21, 2018

joyeecheung added dont-land-on-v4.x labels Feb 21, 2018

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

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fs: fix potential segfault in async calls #18811

fs: fix potential segfault in async calls #18811

joyeecheung commented Feb 16, 2018 •

edited

Loading

joyeecheung commented Feb 16, 2018

bnoordhuis left a comment

bnoordhuis Feb 16, 2018

joyeecheung Feb 16, 2018

bnoordhuis Feb 16, 2018

joyeecheung Feb 16, 2018

bnoordhuis Feb 16, 2018

joyeecheung Feb 16, 2018

bnoordhuis Feb 16, 2018

joyeecheung Feb 16, 2018

joyeecheung commented Feb 16, 2018

addaleax left a comment

joyeecheung commented Feb 16, 2018

jasnell left a comment

joyeecheung commented Feb 16, 2018

joyeecheung commented Feb 17, 2018

joyeecheung commented Feb 19, 2018

MylesBorins commented Feb 21, 2018

joyeecheung commented Feb 21, 2018

fs: fix potential segfault in async calls #18811

fs: fix potential segfault in async calls #18811

Conversation

joyeecheung commented Feb 16, 2018 • edited Loading

Checklist

Affected core subsystem(s)

joyeecheung commented Feb 16, 2018

bnoordhuis left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

joyeecheung commented Feb 16, 2018

addaleax left a comment

Choose a reason for hiding this comment

joyeecheung commented Feb 16, 2018

jasnell left a comment

Choose a reason for hiding this comment

joyeecheung commented Feb 16, 2018

joyeecheung commented Feb 17, 2018

joyeecheung commented Feb 19, 2018

MylesBorins commented Feb 21, 2018

joyeecheung commented Feb 21, 2018

joyeecheung commented Feb 16, 2018 •

edited

Loading