From d20000d3316f64e5f59fa4edaed4ffdd420dc347 Mon Sep 17 00:00:00 2001 From: Gerhard Stoebich <18708370+Flarna@users.noreply.github.com> Date: Tue, 18 Feb 2020 14:37:39 +0100 Subject: [PATCH 1/2] async_hooks: avoid resource reuse by FileHandle Wrap reused read_wrap in a unique async resource to ensure that executionAsyncResource() is not ambiguous. --- src/async_wrap.cc | 6 +- src/async_wrap.h | 3 - src/node_file.cc | 7 ++- test/async-hooks/test-filehandle-no-reuse.js | 64 ++++++++++++++++++++ 4 files changed, 71 insertions(+), 9 deletions(-) create mode 100644 test/async-hooks/test-filehandle-no-reuse.js diff --git a/src/async_wrap.cc b/src/async_wrap.cc index b35cdca08a6f87..4d4cd01b5511c0 100644 --- a/src/async_wrap.cc +++ b/src/async_wrap.cc @@ -587,7 +587,7 @@ AsyncWrap::AsyncWrap(Environment* env, provider_type_ = provider; // Use AsyncReset() call to execute the init() callbacks. - AsyncReset(execution_async_id, silent); + AsyncReset(object, execution_async_id, silent); init_hook_ran_ = true; } @@ -653,10 +653,6 @@ void AsyncWrap::EmitDestroy(Environment* env, double async_id) { env->destroy_async_id_list()->push_back(async_id); } -void AsyncWrap::AsyncReset(double execution_async_id, bool silent) { - AsyncReset(object(), execution_async_id, silent); -} - // Generalized call for both the constructor and for handles that are pooled // and reused over their lifetime. This way a new uid can be assigned when // the resource is pulled out of the pool and put back into use. diff --git a/src/async_wrap.h b/src/async_wrap.h index 33bed41a6439c5..34e84fde6d4823 100644 --- a/src/async_wrap.h +++ b/src/async_wrap.h @@ -170,9 +170,6 @@ class AsyncWrap : public BaseObject { double execution_async_id = kInvalidAsyncId, bool silent = false); - void AsyncReset(double execution_async_id = kInvalidAsyncId, - bool silent = false); - // Only call these within a valid HandleScope. v8::MaybeLocal MakeCallback(const v8::Local cb, int argc, diff --git a/src/node_file.cc b/src/node_file.cc index 121fd35f573434..60a56afe766f56 100644 --- a/src/node_file.cc +++ b/src/node_file.cc @@ -381,7 +381,12 @@ int FileHandle::ReadStart() { if (freelist.size() > 0) { read_wrap = std::move(freelist.back()); freelist.pop_back(); - read_wrap->AsyncReset(); + // Use a fresh async resource. + // Lifetime is ensured via AsyncWrap::resource_. + Local resource = Object::New(env()->isolate()); + resource->Set( + env()->context(), env()->handle_string(), read_wrap->object()); + read_wrap->AsyncReset(resource); read_wrap->file_handle_ = this; } else { Local wrap_obj; diff --git a/test/async-hooks/test-filehandle-no-reuse.js b/test/async-hooks/test-filehandle-no-reuse.js new file mode 100644 index 00000000000000..496eac9817f887 --- /dev/null +++ b/test/async-hooks/test-filehandle-no-reuse.js @@ -0,0 +1,64 @@ +'use strict'; + +const common = require('../common'); +const initHooks = require('./init-hooks'); +const { checkInvocations } = require('./hook-checks'); +const fixtures = require('../common/fixtures'); +if (!common.hasCrypto) + common.skip('missing crypto'); +const http2 = require('http2'); +const assert = require('assert'); +const fs = require('fs'); + +// Checks that the async resource is not reused by FileHandle. +// Test is based on parallel\test-http2-respond-file-fd.js. + +const hooks = initHooks(); +hooks.enable(); + +const { + HTTP2_HEADER_CONTENT_TYPE +} = http2.constants; + +const fname = fixtures.path('elipses.txt'); +const fd = fs.openSync(fname, 'r'); + +const server = http2.createServer(); +server.on('stream', (stream) => { + stream.respondWithFD(fd, { + [HTTP2_HEADER_CONTENT_TYPE]: 'text/plain' + }); +}); +server.on('close', common.mustCall(() => fs.closeSync(fd))); +server.listen(0, () => { + const client = http2.connect(`http://localhost:${server.address().port}`); + const req = client.request(); + + req.on('response', common.mustCall()); + req.on('data', () => {}); + req.on('end', common.mustCall(() => { + client.close(); + server.close(); + })); + req.end(); +}); + +process.on('exit', onExit); + +function onExit() { + hooks.disable(); + hooks.sanityCheck(); + const activities = hooks.activities; + + // Verify both invocations + const fsReqs = activities.filter((x) => x.type === 'FSREQCALLBACK'); + assert.ok(fsReqs.length >= 2); + + checkInvocations(fsReqs[0], { init: 1, destroy: 1 }, 'when process exits'); + checkInvocations(fsReqs[1], { init: 1, destroy: 1 }, 'when process exits'); + + // Verify reuse handle has been wrapped + assert.ok(fsReqs[0].handle !== fsReqs[1].handle, 'Resource reused'); + assert.ok(fsReqs[0].handle === fsReqs[1].handle.handle, + 'Resource not wrapped correctly'); +} From 6e677da3811937405cff84295cd6e9fb3529e8d4 Mon Sep 17 00:00:00 2001 From: Gerhard Stoebich <18708370+Flarna@users.noreply.github.com> Date: Tue, 10 Mar 2020 08:43:23 +0100 Subject: [PATCH 2/2] use larger fixture --- test/async-hooks/test-filehandle-no-reuse.js | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/test/async-hooks/test-filehandle-no-reuse.js b/test/async-hooks/test-filehandle-no-reuse.js index 496eac9817f887..7cd3322c1906fd 100644 --- a/test/async-hooks/test-filehandle-no-reuse.js +++ b/test/async-hooks/test-filehandle-no-reuse.js @@ -20,7 +20,8 @@ const { HTTP2_HEADER_CONTENT_TYPE } = http2.constants; -const fname = fixtures.path('elipses.txt'); +// Use large fixture to get several file operations. +const fname = fixtures.path('person-large.jpg'); const fd = fs.openSync(fname, 'r'); const server = http2.createServer();