Skip to content

Delete broken checks for GCC version that break -fstack-protector-strong#478

Open
berrange wants to merge 1 commit intointel:mainfrom
berrange:gcc-version-checks
Open

Delete broken checks for GCC version that break -fstack-protector-strong#478
berrange wants to merge 1 commit intointel:mainfrom
berrange:gcc-version-checks

Conversation

@berrange
Copy link
Contributor

@berrange berrange commented Dec 4, 2025

The expr comparison is performing a string comparison and is thus broken for any GCC version >= 10, preventing use of -fstack-protector-strong

Since GCC 4.9 was released over 9 years ago (Aug 2016), it is thought reasonable to just drop the conditional check and assume -fstack-protector-strong is always available for GCC.

@berrange
Copy link
Contributor Author

Fixes #447

The expr comparison is performing a string comparison and is thus
broken for any GCC version >= 10, preventing use of -fstack-protector-strong

Since GCC 4.9 was released almost 10 years ago (Aug 2016), it is reasonable
to drop the conditional check and assume -fstack-protector-strong is always
available for GCC.

Signed-off-by: Daniel P. Berrangé <berrange@redhat.com>
SDK_NOT_REQUIRED = 1
ifeq ($(wildcard $(TOP_DIR)/buildenv.mk),)
CXXFLAGS ?= -Wnon-virtual-dtor -std=c++14 -fstack-protector -O2 -D_FORTIFY_SOURCE=2 -UDEBUG -DNDEBUG \
CXXFLAGS ?= -Wnon-virtual-dtor -std=c++14 -fstack-protector-strong -O2 -D_FORTIFY_SOURCE=2 -UDEBUG -DNDEBUG \
Copy link
Contributor

Choose a reason for hiding this comment

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

Added as part of security flags review and adjustments in 8814a5e

Copy link
Contributor

Choose a reason for hiding this comment

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

Added as part of security flags review and adjustments in 8814a5e

Copy link
Contributor

Choose a reason for hiding this comment

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

Added as part of security flags review and adjustments in 8814a5e

Copy link
Contributor

Choose a reason for hiding this comment

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

Refactored as part of security flags review and adjustments in 8814a5e --> -fstack-protector-strong set in buildenv.mk

else
COMMON_FLAGS += -fstack-protector-strong
endif
COMMON_FLAGS += -fstack-protector-strong
Copy link
Contributor

Choose a reason for hiding this comment

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

-fstack-protector-strong set in buildenv.mk as part of 8814a5e --> probably no need to set it up here anymore?

else
ENCLAVE_CFLAGS += -fstack-protector-strong
endif
ENCLAVE_CFLAGS += -fstack-protector-strong
Copy link
Contributor

Choose a reason for hiding this comment

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

covered in 8814a5e

Copy link
Contributor

Choose a reason for hiding this comment

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

covered in 8814a5e

Copy link
Contributor

@bgotowal bgotowal left a comment

Choose a reason for hiding this comment

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

Thank you for proposing the changes!

As noted in #447, we've done modifications in similar areas as part of flag usage refactor in 8814a5e.

Yet, I still see some of you changes worth merging as apparently we've not covered all the fixes. Unsure if you're willing to resolve the conflicts to proceed with this PR...

Looking forward to hearing from you!

@berrange
Copy link
Contributor Author

Yet, I still see some of you changes worth merging as apparently we've not covered all the fixes. Unsure if you're willing to resolve the conflicts to proceed with this PR...

Looking forward to hearing from you!

Sure, I'll put this on my to do list to rebase and drop any obsolete pieces.

@bgotowal bgotowal self-assigned this Mar 17, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants