Avoid overriding _FORTIFY_SOURCE if already set#455
Merged
ranisalt merged 2 commits intoranisalt:masterfrom Apr 24, 2025
Merged
Avoid overriding _FORTIFY_SOURCE if already set#455ranisalt merged 2 commits intoranisalt:masterfrom
ranisalt merged 2 commits intoranisalt:masterfrom
Conversation
threema-danilo
commented
Apr 24, 2025
threema-danilo
force-pushed
the
fix-fortify-source
branch
from
09e5b17 to
22bb42d
Compare
ranisalt
reviewed
Apr 24, 2025
threema-danilo
force-pushed
the
fix-fortify-source
branch
2 times, most recently
from
0ef86b0 to
4bbdadc
Compare
threema-danilo
force-pushed
the
fix-fortify-source
branch
3 times, most recently
from
2b9a3f9 to
8332a8d
Compare
Contributor
Author
|
@ranisalt I confirmed that the regression test works as intended: https://github.com/ranisalt/node-argon2/actions/runs/14639636754/job/41078770309?pr=455 The fix (based on your suggestion with the Regex) is now pushed as well. I did a few tweaks, but so far it fails to run on Windows. I'll have to test this on an actual Windows build machine to find the correct incantation. |
threema-danilo
force-pushed
the
fix-fortify-source
branch
from
8332a8d to
6de3406
Compare
If the environment defines flags other than `_FORTIFY_SOURCE=2` (e.g. on Arch Linux, which sets it to 3), compilation fails because the flag cannot be re-defined. ``` make: Entering directory '/tmp/node-argon2/build' CC(target) Release/obj.target/libargon2/argon2/src/opt.o <command-line>: warning: "_FORTIFY_SOURCE" redefined <command-line>: note: this is the location of the previous definition CC(target) Release/obj.target/libargon2/argon2/src/argon2.o <command-line>: warning: "_FORTIFY_SOURCE" redefined <command-line>: note: this is the location of the previous definition CC(target) Release/obj.target/libargon2/argon2/src/blake2/blake2b.o <command-line>: warning: "_FORTIFY_SOURCE" redefined <command-line>: note: this is the location of the previous definition CC(target) Release/obj.target/libargon2/argon2/src/core.o <command-line>: warning: "_FORTIFY_SOURCE" redefined <command-line>: note: this is the location of the previous definition CC(target) Release/obj.target/libargon2/argon2/src/encoding.o <command-line>: warning: "_FORTIFY_SOURCE" redefined <command-line>: note: this is the location of the previous definition CC(target) Release/obj.target/libargon2/argon2/src/thread.o <command-line>: warning: "_FORTIFY_SOURCE" redefined <command-line>: note: this is the location of the previous definition rm -f Release/obj.target/argon2.a Release/obj.target/argon2.a.ar-file-list; mkdir -p `dirname Release/obj.target/argon2.a` ar crs Release/obj.target/argon2.a @Release/obj.target/argon2.a.ar-file-list COPY Release/argon2.a CXX(target) Release/obj.target/argon2/argon2.o <command-line>: error: "_FORTIFY_SOURCE" redefined [-Werror] <command-line>: note: this is the location of the previous definition cc1plus: all warnings being treated as errors make: *** [argon2.target.mk:138: Release/obj.target/argon2/argon2.o] Error 1 make: Leaving directory '/tmp/node-argon2/build' ``` By only setting it conditionally, we can avoid this issue.
threema-danilo
force-pushed
the
fix-fortify-source
branch
from
6de3406 to
003fe87
Compare
Contributor
Author
|
Finally! Finding a string that works equally on Posix and Windows was quite tough. I had to remove the dollar template strings, because the dollar would get interpreted as a variable by bash when enclosed with double quotes, but single quotes aren't supported by Windows. The final solution is a bit less nice, but seems to work. The two remaining checks are due to permission errors, and no merge blockers, from what I can tell. |
Owner
|
Thanks!
That's never simple 😆 |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
If the environment defines flags other than
_FORTIFY_SOURCE=2(e.g. on Arch Linux, which sets it to 3), compilation fails because the flag cannot be re-defined.By only setting it conditionally, we can avoid this issue.
Fixes #454.