linux: don't resolve symlink on executable#508
Conversation
…en installed via homebrew
WalkthroughThe changes introduce a cross-platform utility function for retrieving the current executable path, replacing direct calls to Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant CLI_Command
participant util.Executable
User->>CLI_Command: Run command (e.g., schedule or display version)
CLI_Command->>util.Executable: Call Executable()
util.Executable-->>CLI_Command: Return executable path (platform-specific)
CLI_Command-->>User: Use path in output or systemd unit
Assessment against linked issues
Assessment against linked issues: Out-of-scope changesNo out-of-scope changes found. ✨ Finishing Touches
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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
util/executable_test.go (1)
11-17: Well-structured test that validates the function contract.The test correctly verifies that
util.Executable()returns an absolute, non-empty path without errors. This ensures consistent behaviour across all platforms.Consider adding a platform-specific test to verify that on Linux, the function returns the same result even when the binary is invoked via a symlink, which is the core use case this PR addresses.
util/executable.go (1)
7-12: Clean implementation with good documentation.The function correctly delegates to
os.Executable()for non-Linux platforms, maintaining existing behaviour. The documentation clearly explains the platform differences.Consider updating the comment to be more specific about what "path to the executable as specified in the command line arguments" means (i.e.,
os.Args[0]) for better clarity.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
commands_display.go(2 hunks)schedule_jobs.go(1 hunks)util/executable.go(1 hunks)util/executable_linux.go(1 hunks)util/executable_test.go(1 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (5)
schedule_jobs.go (2)
util/executable_linux.go (1)
Executable(14-28)util/executable.go (1)
Executable(10-12)
util/executable_test.go (2)
util/executable_linux.go (1)
Executable(14-28)util/executable.go (1)
Executable(10-12)
util/executable.go (1)
util/executable_linux.go (1)
Executable(14-28)
commands_display.go (2)
util/executable_linux.go (1)
Executable(14-28)util/executable.go (1)
Executable(10-12)
util/executable_linux.go (1)
util/executable.go (1)
Executable(10-12)
⏰ Context from checks skipped due to timeout of 90000ms (4)
- GitHub Check: Build and test (1.24, windows-latest)
- GitHub Check: Build and test (1.24, ubuntu-latest)
- GitHub Check: Build and test (1.24, macos-latest)
- GitHub Check: Analyze (go)
🔇 Additional comments (9)
schedule_jobs.go (2)
11-11: LGTM: Import addition aligns with the new utility function.The import of the
utilpackage is correctly added to support the newutil.Executable()function.
19-19: LGTM: Correctly replaces os.Executable() with util.Executable().This change implements the PR objective by using the new platform-specific executable path resolution. On Linux, this will use
os.Args[0]to avoid symlink resolution, whilst maintaining the original behaviour on other platforms.util/executable.go (1)
1-2: Correct build constraint for non-Linux platforms.The build constraint
!linuxensures this implementation is used on all platforms except Linux, which aligns with the PR objective.commands_display.go (2)
19-19: LGTM: Import addition supports the new utility function.The import of the
utilpackage is correctly added to support the updated executable path retrieval.
262-267: Excellent error handling for display purposes.The implementation correctly uses
util.Executable()and gracefully handles errors by setting the executable path to "unknown". This is appropriate for diagnostic output where a missing path shouldn't cause the entire version display to fail.util/executable_linux.go (4)
1-2: LGTM: Correct build constraint for Linux-specific code.The build constraint properly restricts this implementation to Linux systems only.
5-9: LGTM: Appropriate imports for the implementation.The imports are minimal and necessary for the functionality.
11-13: LGTM: Clear documentation explaining the Linux-specific behaviour.The comment clearly explains the difference from
os.Executable()and the Linux-specific implementation rationale.
14-28: Consider security implications and edge cases of usingos.Args[0].The implementation correctly avoids symlink resolution by using
os.Args[0], which aligns with the PR objective. However, there are important considerations:
Security consideration:
os.Args[0]can be manipulated by the process invoker, unlikeos.Executable()which uses system calls. This is less secure but might be acceptable for the use case.Edge case: The code should handle the case where
os.Argsslice is empty.Consider adding a guard for empty
os.Argsslice:func Executable() (string, error) { + if len(os.Args) == 0 { + return "", errors.New("no command line arguments available") + } executable := os.Args[0] if len(executable) == 0 { return "", errors.New("executable path is empty") }Please verify if this security trade-off is acceptable for your use case, considering that attackers could potentially manipulate the executable path through
os.Args[0].
Codecov ReportAttention: Patch coverage is
❌ Your patch status has failed because the patch coverage (40.00%) is below the target coverage (70.00%). You can increase the patch coverage or adjust the target coverage. Additional details and impacted files@@ Coverage Diff @@
## master #508 +/- ##
==========================================
- Coverage 79.33% 79.27% -0.06%
==========================================
Files 134 136 +2
Lines 13233 13252 +19
==========================================
+ Hits 10498 10505 +7
- Misses 2317 2326 +9
- Partials 418 421 +3
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
|
1 participant
Linux fix only:
Don't resolve symlink on executable to keep a constant name (the physical path of the binary might change for each version)
Fixes #490