-
Notifications
You must be signed in to change notification settings - Fork 40
chore: Update compiler flags for security enhancements #550
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
Conversation
As title Log: Update compiler flags for security enhancements Bug: https://pms.uniontech.com/bug-view-337059.html
Reviewer's guide (collapsed on small PRs)Reviewer's GuideThis PR strengthens the build’s security posture by adding linker hardening flags to the global C++ compiler flags in the top-level CMake configuration. Flow diagram for updated build configuration with security linker flagsflowchart TD
A[Top-level CMakeLists.txt] --> B[Set QT_VERSION_MAJOR 6]
A --> C[Append security linker flags to CMAKE_CXX_FLAGS]
C -->|Adds| D[-Wl,-z,relro]
C -->|Adds| E[-Wl,-z,now]
B --> F[Add subdirectory deepin-devicemanager]
B --> G[Add subdirectory deepin-devicemanager-server]
C --> F
C --> G
F --> H[Build deepin-devicemanager with hardened linker flags]
G --> I[Build deepin-devicemanager-server with hardened linker flags]
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
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.
Hey there - I've reviewed your changes - here's some feedback:
- These are linker options rather than compiler flags, so consider moving them to
CMAKE_EXE_LINKER_FLAGS/CMAKE_SHARED_LINKER_FLAGSortarget_link_options()instead ofCMAKE_CXX_FLAGSfor clearer intent and correct tool invocation. - Updating
CMAKE_CXX_FLAGSlike this can overwrite or duplicate existing flags; usingstring(APPEND ...)oradd_link_options()(for modern CMake) is safer and scales better with other configuration. - You may want to guard these ELF-specific
-Wl,-z,...flags behind a compiler/platform check so they don’t break builds on non-GNU or non-ELF toolchains.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- These are linker options rather than compiler flags, so consider moving them to `CMAKE_EXE_LINKER_FLAGS`/`CMAKE_SHARED_LINKER_FLAGS` or `target_link_options()` instead of `CMAKE_CXX_FLAGS` for clearer intent and correct tool invocation.
- Updating `CMAKE_CXX_FLAGS` like this can overwrite or duplicate existing flags; using `string(APPEND ...)` or `add_link_options()` (for modern CMake) is safer and scales better with other configuration.
- You may want to guard these ELF-specific `-Wl,-z,...` flags behind a compiler/platform check so they don’t break builds on non-GNU or non-ELF toolchains.Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
deepin pr auto review我来对这个 CMakeLists.txt 的修改进行审查:
改进建议: # 在 project() 声明后添加
if(CMAKE_BUILD_TYPE STREQUAL "Release")
# 添加安全相关的链接选项
# RELRO: 将某些数据段标记为只读,增强安全性
# BIND_NOW: 立即解析所有动态符号,防止延迟绑定攻击
set(CMAKE_EXE_LINKER_FLAGS "${CMAKE_EXE_LINKER_FLAGS} -Wl,-z,relro -Wl,-z,now")
set(CMAKE_SHARED_LINKER_FLAGS "${CMAKE_SHARED_LINKER_FLAGS} -Wl,-z,relro -Wl,-z,now")
endif()这个改进版本:
|
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: lzwind, wangrong1069 The full list of commands accepted by this bot can be found here. DetailsNeeds approval from an approver in each of these files:Approvers can indicate their approval by writing |
|
/merge |
As title
Log: Update compiler flags for security enhancements
Bug: https://pms.uniontech.com/bug-view-337059.html
Summary by Sourcery
Build: