Compile failure on 16.13.2 & 17.4.0 on RHEL8 #41633
Comments
|
test_crypto_engine modified in 16.13.1 - #40481 |
|
That doesn't seem to have worked ... |
|
With the default configuration on my RHEL8 box it works ok. Trying the options listed above |
|
Added this part of the config |
|
after adding this |
|
Ok adding this |
|
Ok, so it's specifically this -fPIE |
|
There's an old comment from a few years ago about |
|
I don't think this has anything to do with RHEL8 as I'd expect the same problem to exist on other Linux flavors as well. The fundamental problem is that a shared object test_crypto_engine.o is compiled. The configuration in node.gyp Line 1496 in 6fc6ba7 However, if -fPIE is set in CFLAGS which applies to all c files which are compiled. This overrides -fPIC and we end up with the error shown. I don't see a way in gyp to control what CFLAGS applies to. Options like cflags! in the node.gyp file don't seem to have any effect. I believe that is because they would apply to the cflags being built up by gyp not the CFLAGS which are added at the end by the definition for cc in gyp. I managed to build test_crypto_engine.o by modifying the target in the node.gyp file to look like ['(OS=="mac" or (OS=="linux" and target_arch=="x64")) and \
node_use_openssl=="true"', {
'targets': [
{
'target_name': 'test_crypto_engine',
'type': 'none',
'actions': [
{
'action_name': 'test_crypto_engine',
'inputs':[],
'outputs': [
'<(PRODUCT_DIR)/test/fixtures/test_crypto_engine.o',
],
'action': [
'cc',
'-c',
'-I',
'deps/openssl/openssl/include',
'-fPIC',
'-Wno-deprecated-declarations',
'test/fixtures/test_crypto_engine.c',
'-o',
'<(PRODUCT_DIR)/test/fixtures/test_crypto_engine.o',
],
},
],
}
], # end targets
}], # end node_use_openssl sectionThis allowed Building the shared object for the test as part of I think the right way to fix it would be to update so that there is a configure option that would add -fPIE to just the right places. |
|
@RaisinTen what do you think about this comment:
@FireBurn would adding an option to configure in -fPIE in properly be something you'd be interested in contributing a PR for? |
Yes, I agree. We should probably build the engine only when |
|
@RaisinTen I was also wondering if it might make any sense to move the test to be an addon test. If test_crypto_engine could be built as an add-on then we already have the infra for building a shared object (the addon) and running JavaScript to test with it. I've not looked at the test that uses it to know if that makes any sense though. |
|
@mhdawson yes, that sounds good to me. We already have a very similar setup in https://github.com/nodejs/node/tree/HEAD/test/addons/openssl-client-cert-engine, so we can just copy that test and modify the js file by adding some |
|
Unfortunately, https://github.com/nodejs/node/blob/7752eedcc7db586fb0a7c586908d4405c6408325/test/addons/openssl-client-cert-engine/binding.gyp seems to be built on macOS only, so it might need some more modifications to make it work on other platforms. (could be done in a separate PR) |
Fixes: nodejs#41633 - move test-crypto-engine so that dummy engine is only built if tests are run Signed-off-by: Michael Dawson <[email protected]>
Fixes: nodejs#41633 - move test-crypto-engine so that dummy engine is only built if tests are run Signed-off-by: Michael Dawson <[email protected]>
|
@RaisinTen PR to move test to an addon #41830 |
Fixes: nodejs#41633 Fixes: nodejs#40958 - move test-crypto-engine so that dummy engine is only built if tests are run Signed-off-by: Michael Dawson <[email protected]>
|
Is this being backported to 16.x? Still had the same issue building 16.14.0 |
|
@FireBurn I just tagged the PR with the lts-watch-v16.x label which I think should prompt releasers to pull it into a 16.x release unless it needs a manual backport. |
|
The default policy is for a change to have been in a current release for two weeks before being backported to a LTS release. #41830 hasn't gone into a 17.x release yet, AFAICT. |
Fixes: nodejs#41633 Fixes: nodejs#40958 - move test-crypto-engine so that dummy engine is only built if tests are run Signed-off-by: Michael Dawson <[email protected]> PR-URL: nodejs#41830 Reviewed-By: Tobias Nießen <[email protected]> Reviewed-By: Darshan Sen <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Richard Lau <[email protected]>
Fixes: nodejs#41633 Fixes: nodejs#40958 - move test-crypto-engine so that dummy engine is only built if tests are run Signed-off-by: Michael Dawson <[email protected]> PR-URL: nodejs#41830 Reviewed-By: Tobias Nießen <[email protected]> Reviewed-By: Darshan Sen <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Richard Lau <[email protected]>
Fixes: nodejs#41633 Fixes: nodejs#40958 - move test-crypto-engine so that dummy engine is only built if tests are run Signed-off-by: Michael Dawson <[email protected]> PR-URL: nodejs#41830 Reviewed-By: Tobias Nießen <[email protected]> Reviewed-By: Darshan Sen <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Richard Lau <[email protected]>
Version
16.13.2 & 17.4.0
Platform
RHEL8
Subsystem
No response
What steps will reproduce the bug?
Building nodejs on RHEL8
This happens with v16.13.2 but doesn't happen with v16.13.0
How often does it reproduce? Is there a required condition?
No response
What is the expected behavior?
No response
What do you see instead?
Compile failure
Additional information
Configured with
export CFLAGS="-O3 -march=haswell -fPIE -fstack-protector-all -D_FORTIFY_SOURCE=2"
export CXXFLAGS=${CFLAGS}
export LDFLAGS="-Wl,-z,now -Wl,-z,relro"
BUILDTYPE=Release ./configure --shared-zlib
gcc version 8.5.0 20210514
GNU ld version 2.30-108.el8_5.1
The text was updated successfully, but these errors were encountered: