chore: enable PIE in dynamic builds#341
Conversation
WalkthroughThe Makefile's dynamic-link build flags now include -buildmode=pie in GO_LDFLAGS_DYNAMIC, making dynamic-link builds explicitly position-independent; other flags and build targets are unchanged. (50 words) Changes
Sequence Diagram(s)sequenceDiagram
participant Dev as Developer / CI
participant Make as Makefile
participant Go as go build
Dev->>Make: invoke make target (dynamic-link)
Note right of Make: GO_LDFLAGS_DYNAMIC includes\n`-X Version=... -X GitCommit=... -buildmode=pie`
Make->>Go: go build -tags dynamic -ldflags "$(GO_LDFLAGS_DYNAMIC)" $(COVERAGE_FLAG)
alt build succeeds
Go-->>Make: binary (PIE dynamic)
Make-->>Dev: artifact
else build fails
Go-->>Make: error
Make-->>Dev: failure
end
Estimated code review effort🎯 2 (Simple) | ⏱️ ~8 minutes Poem
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
✨ Finishing touches🧪 Generate unit tests
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🧪 Early access (Sonnet 4.5): enabledWe are currently testing the Sonnet 4.5 model, which is expected to improve code review quality. However, this model may lead to increased noise levels in the review comments. Please disable the early access features if the noise level causes any inconvenience. Note:
Comment |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
Makefile (1)
73-73: Good security improvement, but consider consistency with static build.Enabling PIE for dynamic builds is a valuable security enhancement that enables ASLR. The implementation is correct and will work as intended.
However, for better maintainability, consider defining the PIE flag in the
GO_LDFLAGS_DYNAMICvariable (similar to howGO_LDFLAGSincludes-buildmode=pieon line 23) rather than adding it inline. This would make future updates to build flags easier to manage consistently across both build targets.Optional refactor: Apply this diff to align with the static build pattern:
GO_LDFLAGS_DYNAMIC := -X github.com/mozillazg/ptcpdump/internal.Version=$(VERSION) GO_LDFLAGS_DYNAMIC += -X github.com/mozillazg/ptcpdump/internal.GitCommit=$(GIT_COMMIT) +GO_LDFLAGS_DYNAMIC += -buildmode=pieThen update line 73:
- CGO_ENABLED=1 go build -buildmode=pie -tags dynamic -ldflags "$(GO_LDFLAGS_DYNAMIC)" $(COVERAGE_FLAG) + CGO_ENABLED=1 go build -tags dynamic -ldflags "$(GO_LDFLAGS_DYNAMIC)" $(COVERAGE_FLAG)
mozillazg
left a comment
There was a problem hiding this comment.
LGTM, Thanks for your contribution!
This change makes dynamic executables position-independent.
Summary by CodeRabbit