Skip to content

Conversation

@mgovers
Copy link
Member

@mgovers mgovers commented Dec 18, 2025

Cherry-picked from #1228

Investigate the possibilities of hardening

Relates to #1190

Signed-off-by: Martijn Govers <Martijn.Govers@Alliander.com>
Signed-off-by: Martijn Govers <Martijn.Govers@Alliander.com>
Signed-off-by: Martijn Govers <Martijn.Govers@Alliander.com>
Signed-off-by: Martijn Govers <Martijn.Govers@Alliander.com>
Signed-off-by: Martijn Govers <Martijn.Govers@Alliander.com>
Signed-off-by: Martijn Govers <Martijn.Govers@Alliander.com>
@mgovers mgovers added do-not-merge This should not be merged improvement Improvement on internal implementation labels Dec 18, 2025
Signed-off-by: Martijn Govers <Martijn.Govers@Alliander.com>
Signed-off-by: Martijn Govers <Martijn.Govers@Alliander.com>
Signed-off-by: Martijn Govers <Martijn.Govers@Alliander.com>
Signed-off-by: Martijn Govers <Martijn.Govers@Alliander.com>
Signed-off-by: Martijn Govers <Martijn.Govers@Alliander.com>
Comment on lines +34 to +38
set_target_properties(
power_grid_model_api_tests
PROPERTIES BUILD_RPATH $<TARGET_FILE_DIR:power_grid_model_c>
)

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

to be removed

Comment on lines +140 to +142
"environment": {
"SANITIZER_FLAGS": "/RTC1"
},
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

can we move this also to the CMakeLists?

Signed-off-by: Martijn Govers <Martijn.Govers@Alliander.com>
Signed-off-by: Martijn Govers <Martijn.Govers@Alliander.com>

Signed-off-by: Martijn Govers <martygovers@hotmail.com>
Signed-off-by: Martijn Govers <Martijn.Govers@Alliander.com>
Signed-off-by: Martijn Govers <Martijn.Govers@Alliander.com>
Signed-off-by: Martijn Govers <Martijn.Govers@Alliander.com>
Signed-off-by: Martijn Govers <Martijn.Govers@Alliander.com>
Signed-off-by: Martijn Govers <Martijn.Govers@Alliander.com>
Signed-off-by: Martijn Govers <Martijn.Govers@Alliander.com>
Signed-off-by: Martijn Govers <Martijn.Govers@Alliander.com>
Signed-off-by: Martijn Govers <Martijn.Govers@Alliander.com>
Signed-off-by: Martijn Govers <Martijn.Govers@Alliander.com>
Signed-off-by: Martijn Govers <Martijn.Govers@Alliander.com>
project(power_grid_model VERSION ${PGM_VERSION})

option(PGM_ENABLE_DEV_BUILD "Enable developer build (e.g.: tests)" OFF)
option(PGM_ENABLE_HARDENING "Enable compile and link time hardening options" ON)
Copy link
Member

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.

Copy link
Member Author

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

Copy link
Member

@TonyXiang8787 TonyXiang8787 Jan 16, 2026

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.

Copy link
Member Author

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)

This option is intended to be used in production builds, not merely in debug builds.

See also https://www.youtube.com/watch?v=GtYD-AIXBHk&list=PLHTh1InhhwT57vblPGsVag5MkTm_Z9-uq&index=2

Copy link
Member

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.

Copy link
Member Author

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

Comment on lines +214 to +215
before-all = """yum install -y gcc-toolset-14 && \
source /opt/rh/gcc-toolset-14/enable"""
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This should not be needed if we disable build hardening by default.

Signed-off-by: Martijn Govers <Martijn.Govers@Alliander.com>
@mgovers mgovers changed the base branch from main to fix/c-api-opaque-type January 19, 2026 08:43
Signed-off-by: Martijn Govers <martijn.govers@alliander.com>
@sonarqubecloud
Copy link

Base automatically changed from fix/c-api-opaque-type to main January 19, 2026 09:54
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

do-not-merge This should not be merged improvement Improvement on internal implementation

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants