Inherit LDFLAGS for TLS and RDMA modules#3344
Conversation
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## unstable #3344 +/- ##
============================================
+ Coverage 75.05% 75.07% +0.02%
============================================
Files 129 129
Lines 71633 71633
============================================
+ Hits 53763 53781 +18
+ Misses 17870 17852 -18 🚀 New features to boost your workflow:
|
enjoy-binbin
left a comment
There was a problem hiding this comment.
Thanks, please sign the DCO
1ecff16 to
c321a8a
Compare
Commit signed |
Signed-off-by: Remi Collet <remi@remirepo.net>
c321a8a to
0db1a79
Compare
| # valkey-tls.so | ||
| $(TLS_MODULE_NAME): $(SERVER_NAME) | ||
| $(QUIET_CC)$(CC) -o $@ tls.c -shared -fPIC $(TLS_MODULE_CFLAGS) $(TLS_CLIENT_LIBS) | ||
| $(QUIET_CC)$(CC) $(LDFLAGS) -o $@ tls.c -shared -fPIC $(TLS_MODULE_CFLAGS) $(TLS_CLIENT_LIBS) |
There was a problem hiding this comment.
have a small question! is there a reason we used LDFLAGS and not FINAL_LDFLAGS?
There was a problem hiding this comment.
have a small question! is there a reason we used
LDFLAGSand notFINAL_LDFLAGS?
Sorry, I'm not an expert of the very strange valkey build system
I just want to point an issue and propose a minimal fix.
Only LDFLAGS and SERVER_LDFLAGS are documented in the headers
There was a problem hiding this comment.
FINAL_LDFLAGS can include additional optimization and debug flags that are added from other variables:
FINAL_CFLAGS=$(STD) $(WARN) $(OPT) $(DEBUG) $(CFLAGS) $(SERVER_CFLAGS)
FINAL_LDFLAGS=$(LDFLAGS) $(OPT) $(SERVER_LDFLAGS) $(DEBUG)
For compiling, the flags are built up from FINAL_CLFLAGS with some extra defines:
TLS_MODULE_CFLAGS:=$(FINAL_CFLAGS)
...
TLS_MODULE_CFLAGS+=-DUSE_OPENSSL=$(BUILD_MODULE) $(OPENSSL_CFLAGS) -DBUILD_TLS_MODULE=$(BUILD_MODULE)
Maybe we should do the same for link flags, but I don't think any of the debug and optimization flags make any difference for linking, except the -flto flag for link-time-optimization, which needs to be used for compilation and linking to be effective. But the TLS module is built from a single .c file tls.c so there is no link-time-optimization across compilation units anyway.
Bottom line: AFAICT, it doesn't matter if we use LDFLAGS or FINAL_LDFLAGS.
|
@remicollet Do you want this to be backported or is it enough that it will be in 9.1 and later? |
Nice to have in 9.0.x, but we can live without it (and apply it manually in Fedora RPM) |
With current Makefile, `LDFLAGS` are not used for modules. This results in security options not applied. ``` $ annocheck /usr/lib64/valkey/modules/rdma.so annocheck: Version 12.99. Hardened: rdma.so: FAIL: bind-now test because not linked with -Wl,-z,now Hardened: Rerun annocheck with --verbose to see more information on the tests. Hardened: rdma.so: Overall: FAIL. ``` With this patch ``` $ annocheck /usr/lib64/valkey/modules/rdma.so annocheck: Version 12.99. Hardened: rdma.so: PASS. ``` Signed-off-by: Remi Collet <remi@remirepo.net>
With current build system,
LDFLAGSare not used for modulesThis results in security options not applied
With this patch