test: add duplicate symbol test

[gabrielschulhof]

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: nodejs/node-addon-api#456

Checklist

• npm install && npm test passes
• tests are included
• commit message follows commit guidelines

Description of change

[richardlau]

I'm wondering if setting this by default could be breaking for any existing addons?

[gabrielschulhof]

@richardlau addons are not built with -fvisibility=hidden because originally it was impossible to do so, because that would also hide the node::nm_module data symbol that early versions of Node.js were searching for when loading addons. However, it has been possible to load native modules via DSO constructors since 76b9846 (released in v0.11.11), meaning that it would have been possible to introduce -fvisibility=hidden all this time. In fact, in bd8a575 (released in v0.11.12) @bnoordhuis presciently defined NODE_MODULE_EXPORT as __attribute__((visibility("default"))) instead of the empty value it had had up to that point in anticipation of the ability to actually make use of it. Native addons explicitly wishing to expose symbols should prefix them with NODE_MODULE_EXPORT.

It seems that the involuntary exposure of all the symbols has so far caused few problems. Nevertheless, the test in this PR and the issue this PR references indicate that there are problems with unwittingly exposing symbols on OSX.

The problem in the wild can arise from having a large application that has two dependencies, each of which depends on (perhaps a different version of) a native addon. When this addon is loaded from two different places in the dependency tree into the same process, and if this addon exposes symbols using this static inline storage class, then the two different symbols will not be distinguishable on OSX. I have at least one case where that causes a segfault.

I guess the point of discussion should be whether we should takes this opportunity to start building addons with -fvisibility=hidden, at least on OSX, or whether we should take the advocacy approach and evangelize the addition of this flag to each addon's binding.gyp.

Can we test this change with CITGM?

[gabrielschulhof]

@mcollina @kkoopa @addaleax do you think it would be feasible to add -fvisibility=hidden to addon build flags on OSX given the error I found because the flag is absent?

[bnoordhuis]

Thanks Gabriel, LGTM % nits.

[bnoordhuis]

This can go? The add-on doesn't use nan (doesn't even depend on it in package.json.)

[gabrielschulhof]

Actually, it does use nan. I added the dependency to package.json.

[gabrielschulhof]

@bnoordhuis thanks for the review! I have addressed the nits.

[gabrielschulhof]

Rebased to fix conflicts.

[refack]

I'm wondering if setting this by default could be breaking for any existing addons?

Can we test this change with CITGM?

I think we'll need to tweak the CITGM CI script, but @gabrielschulhof you can run CITGM locally on mac. It's as simple as:

npm i -g citgm
citgm-all

But you'll need to transplant a fixed version of node-gyp into npm's node_modules...

[gabrielschulhof]

@refack the Mac machine I have access to is behind a proxy server, so I'm seeing some failures that might be related to that. I'll request access to a CI mac. Let's see if I can run the citgm there.

[gabrielschulhof]

@refack I tried running citgm-all on the Mac to which I received access, but, unfortunately, it fails in lots of places even without replacing node-gyp. Thus, I have no firm basis against which to perform such a test.

[rvagg]

Thus, I have no firm basis against which to perform such a test.
this is the citgm problem, there's no concrete baseline except in the heads of people that use it regularly.

I'm fine with getting this merged but could someone come up with a semver-* label for it? I don't really know. Seems potentially semer-major to me, but maybe not?

[gabrielschulhof]

@rvagg for this to be semver-major there would have to be addons which, upon loading, assume the existence of symbols provided by other addons, symbols which are not explicitly marked NODE_MODULE_EXPORT.

[rvagg]

I changed the commit msg to have a "build,test" prefix and landed in 8b7fdd1. I'm still having trouble with the semveriness of this. semver-minor at least? I don't think I'll include this in 5.0.1 but maybe it should be in a 5.1.0?

[rvagg]

@gabrielschulhof would you mind revisiting this please? I've just noticed the tests fail with Node 12 on macOS, the addon is using a bare GetFunction().

[rvagg]

This works:

diff --git a/test/node_modules/duplicate_symbols/binding.cc b/test/node_modules/duplicate_symbols/binding.cc
index bc5f6f5..a0999b7 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<v8::Object> exports) {
-  exports->Set(Nan::New("pointerCheck").ToLocalChecked(),
-               Nan::New<v8::FunctionTemplate>(Something::PointerCheck)
-      ->GetFunction());
+  Nan::Set(exports, Nan::New("pointerCheck").ToLocalChecked(),
+    Nan::GetFunction(
+      Nan::New<v8::FunctionTemplate>(Something::PointerCheck)).ToLocalChecked());
 }

 NODE_MODULE(NODE_GYP_MODULE_NAME, Init)

since this is already merged, maybe push a new commit here and I'll merge that on top of master.

[rvagg]

oh, and update the "nan" version in package.json when you're in there too I think.

[gabrielschulhof]

@rvagg I made the changes you asked for. I'm not sure if the change I made to the version of nan requested in the test's package.json is the one you had in mind. After all, the existing ^2.0.0 should result in the latest version of nan getting pulled, right?

[rvagg]

great, thanks. and yes you're right about the ^2, my local version was old. landed in af0d7b8