From 2b721345ec2413fa599fc4efec20f9d3bec9852b Mon Sep 17 00:00:00 2001 From: Vincent Weevers Date: Fri, 26 Apr 2019 14:34:04 +0200 Subject: [PATCH 1/4] Prevent segfault: create reference to chained batch object --- binding.cc | 15 +++++++++++--- package.json | 2 +- test/chained-batch-gc-test.js | 37 +++++++++++++++++++++++++++++++++++ 3 files changed, 50 insertions(+), 4 deletions(-) create mode 100644 test/chained-batch-gc-test.js diff --git a/binding.cc b/binding.cc index 0472d1f5..30ad76f5 100644 --- a/binding.cc +++ b/binding.cc @@ -1773,14 +1773,20 @@ NAPI_METHOD(batch_clear) { */ struct BatchWriteWorker final : public PriorityWorker { BatchWriteWorker (napi_env env, + napi_value thisObj, Batch* batch, napi_value callback, bool sync) : PriorityWorker(env, batch->database_, callback, "leveldown.batch.write"), batch_(batch), - sync_(sync) {} + sync_(sync) { + // Prevent GC of batch object before we execute + NAPI_STATUS_THROWS(napi_create_reference(env_, thisObj, 1, &thisRef_)); + } - ~BatchWriteWorker () {} + ~BatchWriteWorker () { + napi_delete_reference(env_, thisRef_); + } void DoExecute () override { SetStatus(batch_->Write(sync_)); @@ -1788,6 +1794,9 @@ struct BatchWriteWorker final : public PriorityWorker { Batch* batch_; bool sync_; + +private: + napi_ref thisRef_; }; /** @@ -1801,7 +1810,7 @@ NAPI_METHOD(batch_write) { bool sync = BooleanProperty(env, options, "sync", false); napi_value callback = argv[2]; - BatchWriteWorker* worker = new BatchWriteWorker(env, batch, callback, sync); + BatchWriteWorker* worker = new BatchWriteWorker(env, argv[0], batch, callback, sync); worker->Queue(); NAPI_RETURN_UNDEFINED(); diff --git a/package.json b/package.json index 87e888cf..84344b02 100644 --- a/package.json +++ b/package.json @@ -7,7 +7,7 @@ "scripts": { "install": "node-gyp-build", "test": "standard && nyc tape test/*-test.js", - "test-gc": "npx -n=--expose-gc tape test/{cleanup,iterator-gc}*-test.js", + "test-gc": "npx -n=--expose-gc tape test/{cleanup,iterator-gc,chained-batch-gc}*-test.js", "test-electron": "electron test/electron.js", "coverage": "nyc report --reporter=text-lcov | coveralls", "rebuild": "node-gyp rebuild", diff --git a/test/chained-batch-gc-test.js b/test/chained-batch-gc-test.js new file mode 100644 index 00000000..1981efcc --- /dev/null +++ b/test/chained-batch-gc-test.js @@ -0,0 +1,37 @@ +'use strict' + +const test = require('tape') +const testCommon = require('./common') + +// When we have a chained batch object without a reference, V8 might GC it +// before we get a chance to (asynchronously) write the batch. +test('chained batch without ref does not get GCed before write', function (t) { + t.plan(2) + + const db = testCommon.factory() + + db.open(function (err) { + t.ifError(err, 'no open error') + + let batch = db.batch() + + for (let i = 0; i < 1e3; i++) { + batch.put(String(i), 'value') + } + + // The sync option makes the operation slower and thus more likely to + // cause a segfault (if the batch were to be GC-ed before it is written). + batch.write({ sync: true }, function (err) { + t.ifError(err, 'no error from write()') + }) + + // Remove reference + batch = null + + if (global.gc) { + // This is the reliable way to trigger GC (and the bug if it exists). + // Useful for manual testing with "node --expose-gc". + global.gc() + } + }) +}) From 5b2e2a6b2bd42867ad913812ea5729b4875bcfce Mon Sep 17 00:00:00 2001 From: Vincent Weevers Date: Fri, 26 Apr 2019 14:39:51 +0200 Subject: [PATCH 2/4] Temporarily repeat tests --- package.json | 2 +- test/chained-batch-gc-test.js | 10 ++++++++-- 2 files changed, 9 insertions(+), 3 deletions(-) diff --git a/package.json b/package.json index 84344b02..f326b92f 100644 --- a/package.json +++ b/package.json @@ -6,7 +6,7 @@ "main": "leveldown.js", "scripts": { "install": "node-gyp-build", - "test": "standard && nyc tape test/*-test.js", + "test": "standard && npm run test-gc && nyc tape test/*-test.js", "test-gc": "npx -n=--expose-gc tape test/{cleanup,iterator-gc,chained-batch-gc}*-test.js", "test-electron": "electron test/electron.js", "coverage": "nyc report --reporter=text-lcov | coveralls", diff --git a/test/chained-batch-gc-test.js b/test/chained-batch-gc-test.js index 1981efcc..86587fce 100644 --- a/test/chained-batch-gc-test.js +++ b/test/chained-batch-gc-test.js @@ -3,9 +3,15 @@ const test = require('tape') const testCommon = require('./common') +function repeat (name, fn) { + for (let i = 0; i < 100; i++) { + test(`${name} (${i})`, fn) + } +} + // When we have a chained batch object without a reference, V8 might GC it // before we get a chance to (asynchronously) write the batch. -test('chained batch without ref does not get GCed before write', function (t) { +repeat('chained batch without ref does not get GCed before write', function (t) { t.plan(2) const db = testCommon.factory() @@ -15,7 +21,7 @@ test('chained batch without ref does not get GCed before write', function (t) { let batch = db.batch() - for (let i = 0; i < 1e3; i++) { + for (let i = 0; i < 1e5; i++) { batch.put(String(i), 'value') } From 3f2916e84262e17dee9223b7d9720f0fd4f55cca Mon Sep 17 00:00:00 2001 From: Vincent Weevers Date: Fri, 26 Apr 2019 15:15:55 +0200 Subject: [PATCH 3/4] Revert "Temporarily repeat tests" This reverts commit 5b2e2a6b2bd42867ad913812ea5729b4875bcfce. --- package.json | 2 +- test/chained-batch-gc-test.js | 10 ++-------- 2 files changed, 3 insertions(+), 9 deletions(-) diff --git a/package.json b/package.json index f326b92f..84344b02 100644 --- a/package.json +++ b/package.json @@ -6,7 +6,7 @@ "main": "leveldown.js", "scripts": { "install": "node-gyp-build", - "test": "standard && npm run test-gc && nyc tape test/*-test.js", + "test": "standard && nyc tape test/*-test.js", "test-gc": "npx -n=--expose-gc tape test/{cleanup,iterator-gc,chained-batch-gc}*-test.js", "test-electron": "electron test/electron.js", "coverage": "nyc report --reporter=text-lcov | coveralls", diff --git a/test/chained-batch-gc-test.js b/test/chained-batch-gc-test.js index 86587fce..1981efcc 100644 --- a/test/chained-batch-gc-test.js +++ b/test/chained-batch-gc-test.js @@ -3,15 +3,9 @@ const test = require('tape') const testCommon = require('./common') -function repeat (name, fn) { - for (let i = 0; i < 100; i++) { - test(`${name} (${i})`, fn) - } -} - // When we have a chained batch object without a reference, V8 might GC it // before we get a chance to (asynchronously) write the batch. -repeat('chained batch without ref does not get GCed before write', function (t) { +test('chained batch without ref does not get GCed before write', function (t) { t.plan(2) const db = testCommon.factory() @@ -21,7 +15,7 @@ repeat('chained batch without ref does not get GCed before write', function (t) let batch = db.batch() - for (let i = 0; i < 1e5; i++) { + for (let i = 0; i < 1e3; i++) { batch.put(String(i), 'value') } From 2ca861befe50b3912336e010da559b354b1f4fca Mon Sep 17 00:00:00 2001 From: Vincent Weevers Date: Fri, 26 Apr 2019 16:30:44 +0200 Subject: [PATCH 4/4] Rename 'thisObj' to 'context' --- binding.cc | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/binding.cc b/binding.cc index 30ad76f5..13a36070 100644 --- a/binding.cc +++ b/binding.cc @@ -1773,7 +1773,7 @@ NAPI_METHOD(batch_clear) { */ struct BatchWriteWorker final : public PriorityWorker { BatchWriteWorker (napi_env env, - napi_value thisObj, + napi_value context, Batch* batch, napi_value callback, bool sync) @@ -1781,11 +1781,11 @@ struct BatchWriteWorker final : public PriorityWorker { batch_(batch), sync_(sync) { // Prevent GC of batch object before we execute - NAPI_STATUS_THROWS(napi_create_reference(env_, thisObj, 1, &thisRef_)); + NAPI_STATUS_THROWS(napi_create_reference(env_, context, 1, &contextRef_)); } ~BatchWriteWorker () { - napi_delete_reference(env_, thisRef_); + napi_delete_reference(env_, contextRef_); } void DoExecute () override { @@ -1796,7 +1796,7 @@ struct BatchWriteWorker final : public PriorityWorker { bool sync_; private: - napi_ref thisRef_; + napi_ref contextRef_; }; /**