Skip to content
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

GHA CI: enable run-time checks in C++ library #19608

Merged
merged 1 commit into from
Sep 18, 2023

Conversation

Chocobo1
Copy link
Member

@Chocobo1 Chocobo1 added the CI Issues/PRs related to CI label Sep 17, 2023
@xavier2k6
Copy link
Member

Did you do any testing with -D_FORTIFY_SOURCE=3?

I came across arvidn/libtorrent#7519 which references below:

@Chocobo1
Copy link
Member Author

Chocobo1 commented Sep 17, 2023

Did you do any testing with -D_FORTIFY_SOURCE=3?

I haven't really checked but I think the compiler/library in CI isn't modern enough to support -D_FORTIFY_SOURCE=3.

@glassez
Copy link
Member

glassez commented Sep 17, 2023

Did you do any testing with -D_FORTIFY_SOURCE=3?

According to docs:

Fortification at this level may have a impact on program performance

I would not add to the CI build configurations anything that could significantly reduce performance or add any significant differences from the release builds, as this may give false/unsuitable test results.

@xavier2k6
Copy link
Member

Fortification at this level may have a impact on program performance

https://siddhesh.in/posts/fortify-source-3-performance.html

@Chocobo1
Copy link
Member Author

Chocobo1 commented Sep 17, 2023

I would not add to the CI build configurations anything that could significantly reduce performance

I agree with the significantly part. Talking about D_FORTIFY_SOURCE=2, I'm not aware users on those distros are reporting such problems and nor do I have ever notice anything from the flag.
Also performance is not a top priority for CI binaries, instead, it is correctness and catching bugs.

_FORTIFY_SOURCE=2 is already in use in ubuntu (for their official software packages): https://wiki.ubuntu.com/ToolChain/CompilerFlags#A-D_FORTIFY_SOURCE.3D2
and fedora: https://fedoraproject.org/wiki/Changes/Add_FORTIFY_SOURCE%3D3_to_distribution_build_flags#Detailed_Description

or add any significant differences from the release builds, as this may give false/unsuitable test results.

I would rather expect the flag to expose valid bugs than to worry about negative side effects.

Also there are other security hardening flags but they won't help catching bugs so I didn't bother. (those are better suited for final release builds)

@Chocobo1 Chocobo1 merged commit 88d32d5 into qbittorrent:master Sep 18, 2023
13 checks passed
@Chocobo1 Chocobo1 deleted the harden branch September 18, 2023 16:25
@xavier2k6
Copy link
Member

In case anyone is interested, I've used (D_FORTIFY_SOURCE=3) via 24.04 image in https://github.com/xavier2k6/qBittorrent/actions/runs/11084022076

@Chocobo1
Copy link
Member Author

In case anyone is interested, I've used (D_FORTIFY_SOURCE=3) via 24.04 image in https://github.com/xavier2k6/qBittorrent/actions/runs/11084022076

👍 Please make a PR.

@xavier2k6
Copy link
Member

Please make a PR.

Currently GHA are migrating ubuntu-latest to use the 24.04 image by default, this is happening from 25th Sept -> Oct 30th.

See: actions/runner-images#10636

As soon as this is done/qBittorrent repo is migrated to use these runners- will open a PR then.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
CI Issues/PRs related to CI
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants