From fac0992c285686dd51ab37478d031674003972c1 Mon Sep 17 00:00:00 2001 From: Christian Niederer Date: Tue, 11 Feb 2020 16:27:19 +0100 Subject: [PATCH 1/3] src: Check for overflow when extending AliasedBufferBase When resizing an aliased_buffer check if the new size will overflow. --- src/aliased_buffer.h | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/src/aliased_buffer.h b/src/aliased_buffer.h index b083fb68e69bd2..44a90250a40d33 100644 --- a/src/aliased_buffer.h +++ b/src/aliased_buffer.h @@ -221,7 +221,8 @@ class AliasedBufferBase { const v8::HandleScope handle_scope(isolate_); const size_t old_size_in_bytes = sizeof(NativeT) * count_; - const size_t new_size_in_bytes = sizeof(NativeT) * new_capacity; + const size_t new_size_in_bytes = MultiplyWithOverflowCheck(sizeof(NativeT), + new_capacity); // allocate v8 new ArrayBuffer v8::Local ab = v8::ArrayBuffer::New( From 2f23066a01fe9dba5ea8b787e4c76625e193d7e8 Mon Sep 17 00:00:00 2001 From: Christian Niederer Date: Wed, 12 Feb 2020 21:07:10 +0100 Subject: [PATCH 2/3] src: Add aliased-buffer-overflow abort test Added native extension similar to test/addons/stringbytes-external-exceeded-max. Added an abort test to regression test the non overflow behaviour. --- test/abort/common.gypi | 8 ++++++ .../binding.cc | 23 +++++++++++++++ .../binding.gyp | 9 ++++++ .../test-abort-aliased-buffer-overflow.js | 28 +++++++++++++++++++ 4 files changed, 68 insertions(+) create mode 100644 test/abort/common.gypi create mode 100644 test/abort/test_abort-aliased-buffer-overflow/binding.cc create mode 100644 test/abort/test_abort-aliased-buffer-overflow/binding.gyp create mode 100644 test/abort/test_abort-aliased-buffer-overflow/test-abort-aliased-buffer-overflow.js diff --git a/test/abort/common.gypi b/test/abort/common.gypi new file mode 100644 index 00000000000000..19396c61856af3 --- /dev/null +++ b/test/abort/common.gypi @@ -0,0 +1,8 @@ +{ + 'defines': [ 'V8_DEPRECATION_WARNINGS=1', 'NODE_WANT_INTERNALS=1' ], + 'conditions': [ + [ 'OS in "linux freebsd openbsd solaris android aix cloudabi"', { + 'cflags': ['-Wno-cast-function-type'], + }], + ], +} diff --git a/test/abort/test_abort-aliased-buffer-overflow/binding.cc b/test/abort/test_abort-aliased-buffer-overflow/binding.cc new file mode 100644 index 00000000000000..c3bf66061bf0ee --- /dev/null +++ b/test/abort/test_abort-aliased-buffer-overflow/binding.cc @@ -0,0 +1,23 @@ +#include +#include +#include + +#include +#include + +void AllocateAndResizeBuffer( + const v8::FunctionCallbackInfo& args) { + v8::Isolate* isolate = args.GetIsolate(); + int64_t length = args[0].As()->Int64Value(); + + node::AliasedBigUint64Array array{isolate, 0}; + + array.reserve(length); + assert(false); + } + +void init(v8::Local exports) { + NODE_SET_METHOD(exports, + "allocateAndResizeBuffer", + AllocateAndResizeBuffer); +} diff --git a/test/abort/test_abort-aliased-buffer-overflow/binding.gyp b/test/abort/test_abort-aliased-buffer-overflow/binding.gyp new file mode 100644 index 00000000000000..55fbe7050f18e4 --- /dev/null +++ b/test/abort/test_abort-aliased-buffer-overflow/binding.gyp @@ -0,0 +1,9 @@ +{ + 'targets': [ + { + 'target_name': 'binding', + 'sources': [ 'binding.cc' ], + 'includes': ['../common.gypi'], + } + ] +} diff --git a/test/abort/test_abort-aliased-buffer-overflow/test-abort-aliased-buffer-overflow.js b/test/abort/test_abort-aliased-buffer-overflow/test-abort-aliased-buffer-overflow.js new file mode 100644 index 00000000000000..33cd21295848b9 --- /dev/null +++ b/test/abort/test_abort-aliased-buffer-overflow/test-abort-aliased-buffer-overflow.js @@ -0,0 +1,28 @@ +'use strict'; +const common = require('../common'); +const assert = require('assert'); +const cp = require('child_process'); + +// This test ensures that during resizing of an Aliased*Array the computation +// of the new size does not overflow. + +if (process.argv[2] === 'child') { + // test + const binding = require(`./build/${common.buildType}/binding`); + + const bigValue = BigInt('0xE000 0000 E000 0000'); + binding.AllocateAndResizeBuffer(bigValue); + assert.fail('this should be unreachable'); +} else { + // observer + const child = cp.spawn(`${process.execPath}`, [`${__filename}`, 'child']); + child.on('exit', common.mustCall(function(code, signal) { + if (common.isWindows) { + assert.strictEqual(code, 134); + assert.strictEqual(signal, null); + } else { + assert.strictEqual(code, null); + assert.strictEqual(signal, 'SIGABRT'); + } + })); +} From 0382006ee8433264789416f1819da0e512eb9580 Mon Sep 17 00:00:00 2001 From: Christian Niederer Date: Fri, 14 Feb 2020 21:16:59 +0100 Subject: [PATCH 3/3] src: Add test/abort build tasks The Aliased*Array overflow check test introduced a dependency to a compiled artifact. Add this artifact to the build process in a similar fashion as test/addons and test/js-native-api do. --- Makefile | 53 +++++++++++++++++++++++++++++++++++++++++++++++------ vcbuild.bat | 30 ++++++++++++++++++++++++++---- 2 files changed, 73 insertions(+), 10 deletions(-) diff --git a/Makefile b/Makefile index 34cdec7f7767b5..3c3bae91c022af 100644 --- a/Makefile +++ b/Makefile @@ -292,7 +292,7 @@ v8: tools/make-v8.sh $(V8_ARCH).$(BUILDTYPE_LOWER) $(V8_BUILD_OPTIONS) .PHONY: jstest -jstest: build-addons build-js-native-api-tests build-node-api-tests ## Runs addon tests and JS tests +jstest: build-addons build-abort-tests build-js-native-api-tests build-node-api-tests ## Runs addon tests and JS tests $(PYTHON) tools/test.py $(PARALLEL_ARGS) --mode=$(BUILDTYPE_LOWER) \ --skip-tests=$(CI_SKIP_TESTS) \ $(CI_JS_SUITES) \ @@ -316,6 +316,7 @@ test: all ## Runs default tests, linters, and builds docs. $(MAKE) -s tooltest $(MAKE) -s test-doc $(MAKE) -s build-addons + $(MAKE) -s build-abort-tests $(MAKE) -s build-js-native-api-tests $(MAKE) -s build-node-api-tests $(MAKE) -s cctest @@ -324,6 +325,7 @@ test: all ## Runs default tests, linters, and builds docs. .PHONY: test-only test-only: all ## For a quick test, does not run linter or build docs. $(MAKE) build-addons + $(MAKE) build-abort-tests $(MAKE) build-js-native-api-tests $(MAKE) build-node-api-tests $(MAKE) cctest @@ -333,6 +335,7 @@ test-only: all ## For a quick test, does not run linter or build docs. # Used by `make coverage-test` test-cov: all $(MAKE) build-addons + $(MAKE) build-abort-tests $(MAKE) build-js-native-api-tests $(MAKE) build-node-api-tests $(MAKE) cctest @@ -452,6 +455,31 @@ test/node-api/.buildstamp: $(ADDONS_PREREQS) \ # TODO(bnoordhuis) Force rebuild after gyp or node-gyp update. build-node-api-tests: | $(NODE_EXE) test/node-api/.buildstamp +ABORT_BINDING_GYPS := \ + $(filter-out test/abort/??_*/binding.gyp, \ + $(wildcard test/abort/*/binding.gyp)) + +ABORT_BINDING_SOURCES := \ + $(filter-out test/abort/??_*/*.c, $(wildcard test/abort/*/*.c)) \ + $(filter-out test/abort/??_*/*.cc, $(wildcard test/abort/*/*.cc)) \ + $(filter-out test/abort/??_*/*.h, $(wildcard test/abort/*/*.h)) + +# Implicitly depends on $(NODE_EXE), see the build-node-api-tests rule for rationale. +test/abort/.buildstamp: $(ADDONS_PREREQS) \ + $(ABORT_BINDING_GYPS) $(ABORT_BINDING_SOURCES) \ + src/node_api.h src/node_api_types.h src/js_native_api.h \ + src/js_native_api_types.h src/js_native_api_v8.h src/js_native_api_v8_internals.h + @$(call run_build_addons,"$$PWD/test/abort",$@) + +.PHONY: build-abort-tests +# .buildstamp needs $(NODE_EXE) but cannot depend on it +# directly because it calls make recursively. The parent make cannot know +# if the subprocess touched anything so it pessimistically assumes that +# .buildstamp is out of date and need a rebuild. +# Just goes to show that recursive make really is harmful... +# TODO(bnoordhuis) Force rebuild after gyp or node-gyp update. +build-abort-tests: | $(NODE_EXE) test/abort/.buildstamp + BENCHMARK_NAPI_BINDING_GYPS := $(wildcard benchmark/napi/*/binding.gyp) BENCHMARK_NAPI_BINDING_SOURCES := \ @@ -472,12 +500,14 @@ clear-stalled: echo $${PS_OUT} | xargs kill -9; \ fi -test-build: | all build-addons build-js-native-api-tests build-node-api-tests +test-build: | all build-addons build-abort-tests build-js-native-api-tests build-node-api-tests test-build-js-native-api: all build-js-native-api-tests test-build-node-api: all build-node-api-tests +test-build-abort: all build-abort-tests + .PHONY: test-all test-all: test-build ## Run default tests with both Debug and Release builds. $(PYTHON) tools/test.py $(PARALLEL_ARGS) --mode=debug,release @@ -490,7 +520,7 @@ test-all-suites: | clear-stalled test-build bench-addons-build doc-only ## Run a $(PYTHON) tools/test.py $(PARALLEL_ARGS) --mode=$(BUILDTYPE_LOWER) test/* # CI_* variables should be kept synchronized with the ones in vcbuild.bat -CI_NATIVE_SUITES ?= addons js-native-api node-api +CI_NATIVE_SUITES ?= addons js-native-api node-api abort CI_JS_SUITES ?= default ifeq ($(node_use_openssl), false) CI_DOC := doctool @@ -502,7 +532,7 @@ endif # Build and test addons without building anything else # Related CI job: node-test-commit-arm-fanned test-ci-native: LOGLEVEL := info -test-ci-native: | test/addons/.buildstamp test/js-native-api/.buildstamp test/node-api/.buildstamp +test-ci-native: | test/addons/.buildstamp test/js-native-api/.buildstamp test/node-api/.buildstamp test/abort/.buildstamp $(PYTHON) tools/test.py $(PARALLEL_ARGS) -p tap --logfile test.tap \ --mode=$(BUILDTYPE_LOWER) --flaky-tests=$(FLAKY_TESTS) \ $(TEST_CI_ARGS) $(CI_NATIVE_SUITES) @@ -524,7 +554,7 @@ test-ci-js: | clear-stalled .PHONY: test-ci # Related CI jobs: most CI tests, excluding node-test-commit-arm-fanned test-ci: LOGLEVEL := info -test-ci: | clear-stalled build-addons build-js-native-api-tests build-node-api-tests doc-only +test-ci: | clear-stalled build-addons build-abort-tests build-js-native-api-tests build-node-api-tests doc-only out/Release/cctest --gtest_output=xml:out/junit/cctest.xml $(PYTHON) tools/test.py $(PARALLEL_ARGS) -p tap --logfile test.tap \ --mode=$(BUILDTYPE_LOWER) --flaky-tests=$(FLAKY_TESTS) \ @@ -628,8 +658,17 @@ test-node-api-clean: $(RM) -r test/node-api/*/build $(RM) test/node-api/.buildstamp +.PHONY: test-abort +test-abort: test-build-abort + $(PYTHON) tools/test.py $(PARALLEL_ARGS) --mode=$(BUILDTYPE_LOWER) test-abort + +.PHONY: test-abort-clean +test-abort-clean: + $(RM) -r test/abort/*/build + $(RM) test/abort/.buildstamp + .PHONY: test-addons -test-addons: test-build test-js-native-api test-node-api +test-addons: test-build test-js-native-api test-node-api test-abort $(PYTHON) tools/test.py $(PARALLEL_ARGS) --mode=$(BUILDTYPE_LOWER) addons .PHONY: test-addons-clean @@ -639,6 +678,7 @@ test-addons-clean: $(RM) test/addons/.buildstamp test/addons/.docbuildstamp $(MAKE) test-js-native-api-clean $(MAKE) test-node-api-clean + $(MAKE) test-abort-clean test-async-hooks: $(PYTHON) tools/test.py $(PARALLEL_ARGS) --mode=$(BUILDTYPE_LOWER) async-hooks @@ -647,6 +687,7 @@ test-with-async-hooks: $(MAKE) build-addons $(MAKE) build-js-native-api-tests $(MAKE) build-node-api-tests + $(MAKE) build-abort-tests $(MAKE) cctest NODE_TEST_WITH_ASYNC_HOOKS=1 $(PYTHON) tools/test.py $(PARALLEL_ARGS) --mode=$(BUILDTYPE_LOWER) \ $(CI_JS_SUITES) \ diff --git a/vcbuild.bat b/vcbuild.bat index 561eb535f2965e..4956a4ac2ca482 100644 --- a/vcbuild.bat +++ b/vcbuild.bat @@ -16,11 +16,11 @@ if /i "%1"=="/?" goto help cd %~dp0 @rem CI_* variables should be kept synchronized with the ones in Makefile -set CI_NATIVE_SUITES=addons js-native-api node-api +set CI_NATIVE_SUITES=addons js-native-api node-api abort set CI_JS_SUITES=default set CI_DOC=doctool @rem Same as the test-ci target in Makefile -set "common_test_suites=%CI_JS_SUITES% %CI_NATIVE_SUITES% %CI_DOC%&set build_addons=1&set build_js_native_api_tests=1&set build_node_api_tests=1" +set "common_test_suites=%CI_JS_SUITES% %CI_NATIVE_SUITES% %CI_DOC%&set build_addons=1&set build_js_native_api_tests=1&set build_node_api_tests=1&set build_aborts_tests=1" @rem Process arguments. set config=Release @@ -68,6 +68,7 @@ set openssl_no_asm= set doc= set extra_msbuild_args= set exit_code=0 +set build_aborts_tests= :next-arg if "%1"=="" goto args-done @@ -96,6 +97,8 @@ if /i "%1"=="test-ci-js" set test_args=%test_args% %test_ci_args% -J -p tap - if /i "%1"=="build-addons" set build_addons=1&goto arg-ok if /i "%1"=="build-js-native-api-tests" set build_js_native_api_tests=1&goto arg-ok if /i "%1"=="build-node-api-tests" set build_node_api_tests=1&goto arg-ok +if /i "%1"=="build-abort-tests" set build_abort_tests=1&goto arg-ok +if /i "%1"=="test-abort" set test_args=%test_args% abort&set build_abort_tests=1&goto arg-ok if /i "%1"=="test-addons" set test_args=%test_args% addons&set build_addons=1&goto arg-ok if /i "%1"=="test-js-native-api" set test_args=%test_args% js-native-api&set build_js_native_api_tests=1&goto arg-ok if /i "%1"=="test-node-api" set test_args=%test_args% node-api&set build_node_api_tests=1&goto arg-ok @@ -585,10 +588,10 @@ endlocal goto build-node-api-tests :build-node-api-tests -if not defined build_node_api_tests goto run-tests +if not defined build_node_api_tests goto build-abort-tests if not exist "%node_exe%" ( echo Failed to find node.exe - goto run-tests + goto build-abort-tests ) echo Building node-api :: clear @@ -601,6 +604,25 @@ set npm_config_nodedir=%~dp0 "%node_exe%" "%~dp0tools\build-addons.js" "%~dp0deps\npm\node_modules\node-gyp\bin\node-gyp.js" "%~dp0test\node-api" if errorlevel 1 exit /b 1 endlocal +goto build-abort-tests + +:build-abort-tests +if not defined build_abort_tests goto run-tests +if not exist "%node_exe%" ( + echo Failed to find node.exe + goto run-tests +) +echo Building abort +:: clear +for /d %%F in (test\abort\??_*) do ( + rd /s /q %%F +) +:: building abort +setlocal +set npm_config_nodedir=%~dp0 +"%node_exe%" "%~dp0tools\build-addons.js" "%~dp0deps\npm\node_modules\node-gyp\bin\node-gyp.js" "%~dp0test\abort" +if errorlevel 1 exit /b 1 +endlocal goto run-tests :run-tests