fs: fix memory corruption issues around fs.Dir #33274

addaleax · 2020-05-07T01:10:37Z

fs: clean up Dir.read() uv_fs_t data before calling into JS

A call into JS can schedule another operation on the same uv_dir_t.
In particular, when the handle is closed from the callback for a
directory read operation, there previously was a race condition window:

A dir.read() operation is submitted to libuv
The read operation is finished by libuv, calling AfterDirRead()
We call into JS
JS calls dir.close()
libuv posts the close request to a thread in the pool
The close request runs, destroying the directory handle
AfterDirRead() is being exited.

Exiting the FSReqAfterScope in step 7 attempts to destroy the original
uv_fs_tfrom step 1, which now points to anuv_dir_t` that has
already been destroyed in step 5.

By forcing the FSReqAfterScope to clean up before we call into JS,
we can be sure that no other operations on the same uv_dir_t are
submitted concurrently.

This addresses issues observed when running with ASAN/valgrind.

fs: forbid concurrent operations on Dir handle

libuv does not expect concurrent operations on uv_dir_t instances,
and will gladly create memory leaks, corrupt data, or crash the
process.

This patch forbids that, and:

Makes sure that concurrent async operations are run sequentially
Throws an exception if sync operations are attempted during an
async operation

The assumption here is that a thrown exception is preferable to
a potential hard crash.

This fully fixes flakiness from parallel/test-fs-opendir when
run under ASAN.

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

A call into JS can schedule another operation on the same `uv_dir_t`. In particular, when the handle is closed from the callback for a directory read operation, there previously was a race condition window: 1. A `dir.read()` operation is submitted to libuv 2. The read operation is finished by libuv, calling `AfterDirRead()` 3. We call into JS 4. JS calls dir.close() 5. libuv posts the close request to a thread in the pool 6. The close request runs, destroying the directory handle 7. `AfterDirRead()` is being exited. Exiting the `FSReqAfterScope` in step 7 attempts to destroy the original uv_fs_t` from step 1, which now points to an `uv_dir_t` that has already been destroyed in step 5. By forcing the `FSReqAfterScope` to clean up before we call into JS, we can be sure that no other operations on the same `uv_dir_t` are submitted concurrently. This addresses issues observed when running with ASAN/valgrind.

libuv does not expect concurrent operations on `uv_dir_t` instances, and will gladly create memory leaks, corrupt data, or crash the process. This patch forbids that, and: - Makes sure that concurrent async operations are run sequentially - Throws an exception if sync operations are attempted during an async operation The assumption here is that a thrown exception is preferable to a potential hard crash. This fully fixes flakiness from `parallel/test-fs-opendir` when run under ASAN.

nodejs-github-bot · 2020-05-07T01:11:30Z

CI: https://ci.nodejs.org/job/node-test-pull-request/31197/ (:yellow_heart:)

jasnell · 2020-05-07T01:20:29Z

Overall, it looks fine. I'm a bit conflicted on the new error code being specific to ERR_DIR_... although I cannot think right now of another place in core where this specific kind of error is currently relevant. Definitely not worth blocking on that but overall a more generalized error code would be better I think.

addaleax · 2020-05-07T01:51:50Z

@jasnell I would agree, but our errors codes are too specific in general, so at least this is consistent 😬

addaleax · 2020-05-09T02:38:39Z

@nodejs/fs

addaleax · 2020-05-15T17:37:11Z

Landed in 441e703...d3a8a23

A call into JS can schedule another operation on the same `uv_dir_t`. In particular, when the handle is closed from the callback for a directory read operation, there previously was a race condition window: 1. A `dir.read()` operation is submitted to libuv 2. The read operation is finished by libuv, calling `AfterDirRead()` 3. We call into JS 4. JS calls dir.close() 5. libuv posts the close request to a thread in the pool 6. The close request runs, destroying the directory handle 7. `AfterDirRead()` is being exited. Exiting the `FSReqAfterScope` in step 7 attempts to destroy the original uv_fs_t` from step 1, which now points to an `uv_dir_t` that has already been destroyed in step 5. By forcing the `FSReqAfterScope` to clean up before we call into JS, we can be sure that no other operations on the same `uv_dir_t` are submitted concurrently. This addresses issues observed when running with ASAN/valgrind. PR-URL: #33274 Reviewed-By: Colin Ihrig <[email protected]>

libuv does not expect concurrent operations on `uv_dir_t` instances, and will gladly create memory leaks, corrupt data, or crash the process. This patch forbids that, and: - Makes sure that concurrent async operations are run sequentially - Throws an exception if sync operations are attempted during an async operation The assumption here is that a thrown exception is preferable to a potential hard crash. This fully fixes flakiness from `parallel/test-fs-opendir` when run under ASAN. PR-URL: #33274 Reviewed-By: Colin Ihrig <[email protected]>

A call into JS can schedule another operation on the same `uv_dir_t`. In particular, when the handle is closed from the callback for a directory read operation, there previously was a race condition window: 1. A `dir.read()` operation is submitted to libuv 2. The read operation is finished by libuv, calling `AfterDirRead()` 3. We call into JS 4. JS calls dir.close() 5. libuv posts the close request to a thread in the pool 6. The close request runs, destroying the directory handle 7. `AfterDirRead()` is being exited. Exiting the `FSReqAfterScope` in step 7 attempts to destroy the original uv_fs_t` from step 1, which now points to an `uv_dir_t` that has already been destroyed in step 5. By forcing the `FSReqAfterScope` to clean up before we call into JS, we can be sure that no other operations on the same `uv_dir_t` are submitted concurrently. This addresses issues observed when running with ASAN/valgrind. PR-URL: #33274 Reviewed-By: Colin Ihrig <[email protected]>

libuv does not expect concurrent operations on `uv_dir_t` instances, and will gladly create memory leaks, corrupt data, or crash the process. This patch forbids that, and: - Makes sure that concurrent async operations are run sequentially - Throws an exception if sync operations are attempted during an async operation The assumption here is that a thrown exception is preferable to a potential hard crash. This fully fixes flakiness from `parallel/test-fs-opendir` when run under ASAN. PR-URL: #33274 Reviewed-By: Colin Ihrig <[email protected]>

A call into JS can schedule another operation on the same `uv_dir_t`. In particular, when the handle is closed from the callback for a directory read operation, there previously was a race condition window: 1. A `dir.read()` operation is submitted to libuv 2. The read operation is finished by libuv, calling `AfterDirRead()` 3. We call into JS 4. JS calls dir.close() 5. libuv posts the close request to a thread in the pool 6. The close request runs, destroying the directory handle 7. `AfterDirRead()` is being exited. Exiting the `FSReqAfterScope` in step 7 attempts to destroy the original uv_fs_t` from step 1, which now points to an `uv_dir_t` that has already been destroyed in step 5. By forcing the `FSReqAfterScope` to clean up before we call into JS, we can be sure that no other operations on the same `uv_dir_t` are submitted concurrently. This addresses issues observed when running with ASAN/valgrind. PR-URL: #33274 Reviewed-By: Colin Ihrig <[email protected]>

libuv does not expect concurrent operations on `uv_dir_t` instances, and will gladly create memory leaks, corrupt data, or crash the process. This patch forbids that, and: - Makes sure that concurrent async operations are run sequentially - Throws an exception if sync operations are attempted during an async operation The assumption here is that a thrown exception is preferable to a potential hard crash. This fully fixes flakiness from `parallel/test-fs-opendir` when run under ASAN. PR-URL: #33274 Reviewed-By: Colin Ihrig <[email protected]>

A call into JS can schedule another operation on the same `uv_dir_t`. In particular, when the handle is closed from the callback for a directory read operation, there previously was a race condition window: 1. A `dir.read()` operation is submitted to libuv 2. The read operation is finished by libuv, calling `AfterDirRead()` 3. We call into JS 4. JS calls dir.close() 5. libuv posts the close request to a thread in the pool 6. The close request runs, destroying the directory handle 7. `AfterDirRead()` is being exited. Exiting the `FSReqAfterScope` in step 7 attempts to destroy the original uv_fs_t` from step 1, which now points to an `uv_dir_t` that has already been destroyed in step 5. By forcing the `FSReqAfterScope` to clean up before we call into JS, we can be sure that no other operations on the same `uv_dir_t` are submitted concurrently. This addresses issues observed when running with ASAN/valgrind. PR-URL: nodejs#33274 Reviewed-By: Colin Ihrig <[email protected]>

libuv does not expect concurrent operations on `uv_dir_t` instances, and will gladly create memory leaks, corrupt data, or crash the process. This patch forbids that, and: - Makes sure that concurrent async operations are run sequentially - Throws an exception if sync operations are attempted during an async operation The assumption here is that a thrown exception is preferable to a potential hard crash. This fully fixes flakiness from `parallel/test-fs-opendir` when run under ASAN. PR-URL: nodejs#33274 Reviewed-By: Colin Ihrig <[email protected]>

addaleax added 2 commits May 7, 2020 03:05

nodejs-github-bot added c++ Issues and PRs that require attention from people who are familiar with C++. lib / src Issues and PRs related to general changes in the lib or src directory. labels May 7, 2020

addaleax added fs Issues and PRs related to the fs subsystem / file system. and removed lib / src Issues and PRs related to general changes in the lib or src directory. labels May 7, 2020

addaleax mentioned this pull request May 7, 2020

build: run full test suite in ASAN action #33170

Closed

2 tasks

addaleax added the review wanted PRs that need reviews. label May 7, 2020

cjihrig approved these changes May 9, 2020

View reviewed changes

addaleax added author ready PRs that have at least one approval, no pending requests for changes, and a CI started. and removed review wanted PRs that need reviews. labels May 15, 2020

addaleax closed this May 15, 2020

addaleax deleted the dir-clear-before-js branch May 15, 2020 17:37

codebytere mentioned this pull request May 18, 2020

v14.3.0 proposal #33452

Merged

codebytere mentioned this pull request Jun 9, 2020

v12.18.1 proposal #33811

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fs: fix memory corruption issues around fs.Dir #33274

fs: fix memory corruption issues around fs.Dir #33274

addaleax commented May 7, 2020

nodejs-github-bot commented May 7, 2020 •

edited by addaleax

Loading

jasnell commented May 7, 2020

addaleax commented May 7, 2020

addaleax commented May 9, 2020

addaleax commented May 15, 2020

fs: fix memory corruption issues around fs.Dir #33274

fs: fix memory corruption issues around fs.Dir #33274

Conversation

addaleax commented May 7, 2020

fs: clean up Dir.read() uv_fs_t data before calling into JS

fs: forbid concurrent operations on Dir handle

Checklist

nodejs-github-bot commented May 7, 2020 • edited by addaleax Loading

jasnell commented May 7, 2020

addaleax commented May 7, 2020

addaleax commented May 9, 2020

addaleax commented May 15, 2020

nodejs-github-bot commented May 7, 2020 •

edited by addaleax

Loading