-
Notifications
You must be signed in to change notification settings - Fork 54
OpenSSF silver badge: hardening #1241
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Changes from all commits
147a479
2c7c085
a482a73
43b2aa6
0a6a25e
d95c9d3
ffdac64
70c0d1a
8c718b4
44ebe4c
b026dad
c2af03b
b9b19e0
00a23d7
422112b
036a32c
4ca6f14
7507da7
46f8519
2258e0c
f87f5c3
2f4597b
da8a3dd
53e2856
a703a3b
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -112,7 +112,7 @@ | |
| { | ||
| "name": "unix-sanitizer", | ||
| "environment": { | ||
| "SANITIZER_FLAGS": "-fsanitize=address" | ||
| "SANITIZER_FLAGS": "-fstack-protector -fsanitize=address,pointer-compare,undefined -fno-sanitize-recover" | ||
| }, | ||
| "inherits": "unix-base", | ||
| "hidden": true | ||
|
|
@@ -140,6 +140,9 @@ | |
| { | ||
| "name": "msvc-debug", | ||
| "displayName": "Debug (MSVC)", | ||
| "environment": { | ||
| "SANITIZER_FLAGS": "/RTC1" | ||
| }, | ||
|
Comment on lines
+143
to
+145
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. can we move this also to the CMakeLists? |
||
| "inherits": [ | ||
| "msvc-base", | ||
| "debug" | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -76,11 +76,13 @@ Discussion = "https://github.com/orgs/PowerGridModel/discussions" | |
| power_grid_model = "power_grid_model._core.power_grid_model_c" | ||
|
|
||
| [tool.scikit-build] | ||
| logging.level = "INFO" | ||
| logging.level = "DEBUG" | ||
|
|
||
| cmake.version = ">=3.23" | ||
| cmake.build-type = "Release" | ||
| cmake.args = ["-GNinja"] | ||
| cmake.verbose = true | ||
|
|
||
|
|
||
| build-dir = "build" | ||
|
|
||
|
|
@@ -204,11 +206,12 @@ test-command = "pytest {package}/tests" | |
| # we do not support | ||
| # PyPy | ||
| # musllinux in aarch64 | ||
| # disable Python 3.14 for now until it is released | ||
| skip = ["pp*", "*-musllinux_aarch64"] | ||
|
|
||
| [tool.cibuildwheel.linux] | ||
| archs = ["x86_64", "aarch64"] | ||
| before-all = """yum install -y gcc-toolset-14 && \ | ||
| source /opt/rh/gcc-toolset-14/enable""" | ||
|
Comment on lines
+213
to
+214
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This should not be needed if we disable build hardening by default. |
||
| environment = { CC = "gcc", CXX = "g++" } | ||
| manylinux-x86_64-image = "manylinux_2_28" | ||
| manylinux-aarch64-image = "manylinux_2_28" | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -31,4 +31,9 @@ target_compile_definitions( | |
| PRIVATE PGM_ENABLE_EXPERIMENTAL | ||
| ) | ||
|
|
||
| set_target_properties( | ||
| power_grid_model_api_tests | ||
| PROPERTIES BUILD_RPATH $<TARGET_FILE_DIR:power_grid_model_c> | ||
| ) | ||
|
|
||
|
Comment on lines
+34
to
+38
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. to be removed |
||
| doctest_discover_tests(power_grid_model_api_tests) | ||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think enable it by default is logical. Disable by default and only enable in the our dev preset is logical.
In the Python build we do not enable it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hardening is intended to be used in production
Uh oh!
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The hardened code self (so the adjustment to make compilation to pass with hardening check flag) is intended to be used in production. The compilation flags to check hardening are not intended to be used in production.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
From the GCC command line options (https://gcc.gnu.org/onlinedocs/gcc/Instrumentation-Options.html#index-fhardened)
See also https://www.youtube.com/watch?v=GtYD-AIXBHk&list=PLHTh1InhhwT57vblPGsVag5MkTm_Z9-uq&index=2
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I still has huge doubt for the default. This goes against the other golden rule of open-source cmake project: make as little as needed for extra compile and link flag in the default cmake build.
Also, enabling address sanitizing for production is questionable.
We need more in-depth consideration and discussion.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We can also let CIBW set the compiler flags for hardening