From ef5354c425ea69e049e181d958a4649d3ea4b0ca Mon Sep 17 00:00:00 2001 From: Gabriel Schulhof Date: Mon, 11 Mar 2019 22:14:58 -0700 Subject: [PATCH 1/2] build,test: add duplicate symbol test On OSX symbols are exported by default, and they overlap if two different copies of the same symbol appear in a process. This change ensures that, on OSX, symbols are hidden by default. Re: https://github.com/nodejs/node-addon-api/pull/456 Fixes: https://github.com/nodejs/node/issues/26765 PR-URL: https://github.com/nodejs/node-gyp/pull/1689 Reviewed-By: Ben Noordhuis Reviewed-By: Refael Ackermann --- addon.gypi | 4 ++ .../node_modules/duplicate_symbols/binding.cc | 10 +++++ .../duplicate_symbols/binding.gyp | 19 ++++++++++ test/node_modules/duplicate_symbols/common.h | 37 +++++++++++++++++++ test/node_modules/duplicate_symbols/extra.cc | 6 +++ test/node_modules/duplicate_symbols/index.js | 5 +++ .../duplicate_symbols/package.json | 14 +++++++ test/test-addon.js | 24 ++++++++++++ 8 files changed, 119 insertions(+) create mode 100644 test/node_modules/duplicate_symbols/binding.cc create mode 100644 test/node_modules/duplicate_symbols/binding.gyp create mode 100644 test/node_modules/duplicate_symbols/common.h create mode 100644 test/node_modules/duplicate_symbols/extra.cc create mode 100644 test/node_modules/duplicate_symbols/index.js create mode 100644 test/node_modules/duplicate_symbols/package.json diff --git a/addon.gypi b/addon.gypi index 6462f539ff..3cadaab78b 100644 --- a/addon.gypi +++ b/addon.gypi @@ -90,10 +90,14 @@ 'conditions': [ [ 'OS=="mac"', { + 'cflags': [ + '-fvisibility=hidden' + ], 'defines': [ '_DARWIN_USE_64_BIT_INODE=1' ], 'xcode_settings': { + 'GCC_SYMBOLS_PRIVATE_EXTERN': 'YES', # -fvisibility=hidden 'DYLIB_INSTALL_NAME_BASE': '@rpath' }, }], diff --git a/test/node_modules/duplicate_symbols/binding.cc b/test/node_modules/duplicate_symbols/binding.cc new file mode 100644 index 0000000000..bc5f6f5134 --- /dev/null +++ b/test/node_modules/duplicate_symbols/binding.cc @@ -0,0 +1,10 @@ +#include +#include "common.h" + +void Init(v8::Local exports) { + exports->Set(Nan::New("pointerCheck").ToLocalChecked(), + Nan::New(Something::PointerCheck) + ->GetFunction()); +} + +NODE_MODULE(NODE_GYP_MODULE_NAME, Init) diff --git a/test/node_modules/duplicate_symbols/binding.gyp b/test/node_modules/duplicate_symbols/binding.gyp new file mode 100644 index 0000000000..46f1c8cba4 --- /dev/null +++ b/test/node_modules/duplicate_symbols/binding.gyp @@ -0,0 +1,19 @@ +{ + "target_defaults": { + "include_dirs": [ + " + +class Something { + public: + static void PointerCheck(const Nan::FunctionCallbackInfo& info); +}; + +// Removing the inline keyword below will result in the addon failing to link +// on OSX because of a duplicate symbol. +inline void +Something::PointerCheck(const Nan::FunctionCallbackInfo& info) { + v8::Local v8result; + + if (info.Length() > 0) { + // If an argument was passed in, it is a pointer to the `PointerCheck` + // method from the other addon. So, we compare it to the value of the + // pointer to the `PointerCheck` method in this addon, and return + // `"equal"` if they are equal, and `"not equal"` otherwise". + + const char* result = + (reinterpret_cast(Something::PointerCheck) == + info[0].As()->Value()) ? + "equal" : "not equal"; + v8result = Nan::New(result).ToLocalChecked(); + } else { + // If no argument was passed in, we wrap the pointer to the `PointerCheck` + // method in this addon into a `v8::External` and pass it into JavaScript. + v8result = Nan::New( + reinterpret_cast(Something::PointerCheck)); + } + info.GetReturnValue().Set(v8result); +} + +#endif // DUPLICATE_SYMBOLS_COMMON_H_ diff --git a/test/node_modules/duplicate_symbols/extra.cc b/test/node_modules/duplicate_symbols/extra.cc new file mode 100644 index 0000000000..0c9d5dbebd --- /dev/null +++ b/test/node_modules/duplicate_symbols/extra.cc @@ -0,0 +1,6 @@ +#include "common.h" + +// It is important that common.h be included from two different translation +// units, because doing so can create duplicate symbols in some instances. If +// it does so and fails to build because of it, that is considered a test +// failure. diff --git a/test/node_modules/duplicate_symbols/index.js b/test/node_modules/duplicate_symbols/index.js new file mode 100644 index 0000000000..09dbed81b7 --- /dev/null +++ b/test/node_modules/duplicate_symbols/index.js @@ -0,0 +1,5 @@ +'use strict' +module.exports = { + pointerCheck1: require('bindings')('binding1').pointerCheck, + pointerCheck2: require('bindings')('binding2').pointerCheck +}; diff --git a/test/node_modules/duplicate_symbols/package.json b/test/node_modules/duplicate_symbols/package.json new file mode 100644 index 0000000000..ee21161cf1 --- /dev/null +++ b/test/node_modules/duplicate_symbols/package.json @@ -0,0 +1,14 @@ +{ + "name": "duplicate_symbols", + "version": "0.0.0", + "description": "Duplicate Symbols Test", + "main": "index.js", + "private": true, + "dependencies": { + "bindings": "~1.2.1", + "nan": "^2.0.0" + }, + "scripts": { + "test": "node index.js" + } +} diff --git a/test/test-addon.js b/test/test-addon.js index f79eff73c1..028d98337a 100644 --- a/test/test-addon.js +++ b/test/test-addon.js @@ -23,6 +23,15 @@ function getEncoding () { return execFileSync('python', ['-c', code]).toString().trim() } +function runDuplicateBindings () { + const hostProcess = process.execPath + const testCode = + 'console.log((function(bindings) {' + + 'return bindings.pointerCheck1(bindings.pointerCheck2());' + + "})(require('duplicate_symbols')))" + return execFileSync(hostProcess, ['-e', testCode], { cwd: __dirname }).toString() +} + function checkCharmapValid () { var data try { @@ -51,6 +60,21 @@ test('build simple addon', function (t) { proc.stderr.setEncoding('utf-8') }) +test('make sure addon symbols do not overlap', function (t) { + t.plan(3) + + var addonPath = path.resolve(__dirname, 'node_modules', 'duplicate_symbols') + // Set the loglevel otherwise the output disappears when run via 'npm test' + var cmd = [nodeGyp, 'rebuild', '-C', addonPath, '--loglevel=verbose'] + execFile(process.execPath, cmd, function (err, stdout, stderr) { + var logLines = stderr.trim().split(/\r?\n/) + var lastLine = logLines[logLines.length - 1] + t.strictEqual(err, null) + t.strictEqual(lastLine, 'gyp info ok', 'should end in ok') + t.strictEqual(runDuplicateBindings().trim(), 'not equal') + }) +}) + test('build simple addon in path with non-ascii characters', function (t) { t.plan(1) From c26ca1b5421e21c72406a8cf8c7ac5ed322233c5 Mon Sep 17 00:00:00 2001 From: Gabriel Schulhof Date: Thu, 20 Jun 2019 09:32:40 -0700 Subject: [PATCH 2/2] test: use Nan in duplicate_symbols Use `Nan::Set()` and `Nan::GetFunction()` instead of their V8 equivalents to avoid OSX test failures with Node.js v12.x. Thanks @rvagg! Re: https://github.com/nodejs/node-addon-api/pull/456 Fixes: https://github.com/nodejs/node/issues/26765 PR-URL: https://github.com/nodejs/node-gyp/pull/1689 Reviewed-By: Rod Vagg --- test/node_modules/duplicate_symbols/binding.cc | 6 +++--- test/node_modules/duplicate_symbols/package.json | 2 +- 2 files changed, 4 insertions(+), 4 deletions(-) diff --git a/test/node_modules/duplicate_symbols/binding.cc b/test/node_modules/duplicate_symbols/binding.cc index bc5f6f5134..a0999b76dc 100644 --- a/test/node_modules/duplicate_symbols/binding.cc +++ b/test/node_modules/duplicate_symbols/binding.cc @@ -2,9 +2,9 @@ #include "common.h" void Init(v8::Local exports) { - exports->Set(Nan::New("pointerCheck").ToLocalChecked(), - Nan::New(Something::PointerCheck) - ->GetFunction()); + Nan::Set(exports, Nan::New("pointerCheck").ToLocalChecked(), + Nan::GetFunction( + Nan::New(Something::PointerCheck)).ToLocalChecked()); } NODE_MODULE(NODE_GYP_MODULE_NAME, Init) diff --git a/test/node_modules/duplicate_symbols/package.json b/test/node_modules/duplicate_symbols/package.json index ee21161cf1..2a193f230f 100644 --- a/test/node_modules/duplicate_symbols/package.json +++ b/test/node_modules/duplicate_symbols/package.json @@ -6,7 +6,7 @@ "private": true, "dependencies": { "bindings": "~1.2.1", - "nan": "^2.0.0" + "nan": "^2.14.0" }, "scripts": { "test": "node index.js"