-
Notifications
You must be signed in to change notification settings - Fork 1.6k
Remove useless command calls #454
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
base: main
Are you sure you want to change the base?
Conversation
We don’t need any complicated whoami, getend or awk commands to get the user’s home directory, when std::env::home_dir() already got us covered. This lets us remove the run_cmds_trim_newline() helper as well, since it was the only user of it.
This was a very elaborate way of reading a file into a String, so let’s do that instead.
awk is nice and all, but we can parse that file much more efficiently directly in Rust. This also lets us get rid of the run_cmds() function.
We can invoke the pgrep command directly just fine. Although this whole function seems extremely brittle, kded can perfectly well run outside of KDE, it doesn’t make it a KDE session.
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.
Pull request overview
This PR removes unnecessary shell command invocations in the Linux platform code, replacing them with native Rust implementations. The changes eliminate dependencies on external commands like sh, awk, ps, cat, and whoami, improving performance and reducing potential security issues from shell command injection.
- Replaces shell commands with direct file I/O for reading
/etc/os-releaseand/proc/self/sessionid - Removes unused command path constants (
CMD_PS,CMD_SH) - Replaces
whoami/getentcommands withstd::env::home_dir()for home directory resolution - Simplifies
pgrepinvocation by removing unnecessary shell wrapper
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
| src/platform/linux.rs | Refactors Distro::new() to read /etc/os-release directly, simplifies is_kde_session() pgrep call, replaces cat with fs::read_to_string(), removes unused helper functions and their tests |
| src/config.rs | Replaces whoami/getent command chain with std::env::home_dir() for home directory detection |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| if let Some((key, value)) = line.unwrap_or_default().split_once('=') { | ||
| if key == "NAME" { | ||
| name = Some(value.trim_matches('"').to_owned()); | ||
| } else if key == "VERSION_ID" { | ||
| version_id = Some(value.trim_matches('"').to_owned()); |
Copilot
AI
Dec 12, 2025
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.
The line.unwrap_or_default() call silently ignores I/O errors when reading lines from the file. If a line read fails, it will be treated as an empty string, potentially skipping valid data. Consider using a pattern that either propagates errors or handles them explicitly, such as:
if let Ok(line_content) = line {
if let Some((key, value)) = line_content.split_once('=') {
// process key-value pairs
}
}| if let Some((key, value)) = line.unwrap_or_default().split_once('=') { | |
| if key == "NAME" { | |
| name = Some(value.trim_matches('"').to_owned()); | |
| } else if key == "VERSION_ID" { | |
| version_id = Some(value.trim_matches('"').to_owned()); | |
| if let Ok(line_content) = line { | |
| if let Some((key, value)) = line_content.split_once('=') { | |
| if key == "NAME" { | |
| name = Some(value.trim_matches('"').to_owned()); | |
| } else if key == "VERSION_ID" { | |
| version_id = Some(value.trim_matches('"').to_owned()); | |
| } |
| if let Some(home_dir) = std::env::home_dir() { | ||
| return home_dir; |
Copilot
AI
Dec 12, 2025
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.
The std::env::home_dir() function has been deprecated since Rust 1.29 (2018) due to incorrect behavior on Windows and platform inconsistencies. The deprecation notice recommends using the dirs or dirs_next crate instead. This codebase already uses dirs_next::home_dir() in other places (lines 628, 667), so you should use the same function here for consistency and to avoid using deprecated APIs.
| } | ||
| if session.is_empty() { | ||
| session = run_cmds("cat /proc/self/sessionid").unwrap_or_default(); | ||
| session = std::fs::read_to_string("/proc/self/sessionid").unwrap_or_default(); |
Copilot
AI
Dec 12, 2025
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.
The std::fs::read_to_string() call includes any trailing newline characters from the file. The previous cat /proc/self/sessionid command through the shell would have included the newline, but the comparison with INVALID_SESSION on line 155 might now fail if the file content has trailing whitespace. Consider trimming the result: std::fs::read_to_string("/proc/self/sessionid").unwrap_or_default().trim().to_string()
| session = std::fs::read_to_string("/proc/self/sessionid").unwrap_or_default(); | |
| session = std::fs::read_to_string("/proc/self/sessionid").unwrap_or_default().trim().to_string(); |
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.
This file can’t ever contain any newline, it can only be a decimal integer.
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.
I thinks it's better to always trim() here.
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.
I disagree, this is ABI, the kernel will never ever add a newline or any other character here, so trim() will always be a noop.
That said, in a future PR I have already started working on, this whole mess of fetching the session is simplified by always using logind using zbus.
| lazy_static::lazy_static! { | ||
| pub static ref CMD_LOGINCTL: String = find_cmd_path("loginctl"); | ||
| pub static ref CMD_PS: String = find_cmd_path("ps"); | ||
| pub static ref CMD_SH: String = find_cmd_path("sh"); |
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.
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.
Oh, I was working on this repository alone, and didn’t notice it was API for rustdesk, my bad! I’ll put all of the pub symbols back to their former state then, and also do the same kind of fixes in rustdesk, thanks!
|
|
||
| // **Note** that the return value here, the last character is '\n'. | ||
| // Use `run_cmds_trim_newline()` if you want to remove '\n' at the end. | ||
| pub fn run_cmds(cmds: &str) -> ResultType<String> { |
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.
run_cmds() is commonly used in https://github.com/rustdesk/rustdesk
Maybe we need to keep it in this PR.
| std::process::Command::new(CMD_SH.as_str()) | ||
| .arg("-c") | ||
| .arg("pgrep -f kded[0-9]+") | ||
| Command::new("pgrep") |
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.
Directly removing CMD_SH.as_str() may generate an excessive number of audit logs.
hbb_common/src/platform/linux.rs
Line 26 in 8b0e258
| // `ausearch -x /usr/share/rustdesk/rustdesk` will return |
ausearch -x /usr/share/rustdesk/rustdesk
This is because the audit log will record ENOENT errors when the system attempts to resolve the absolute path of the command.
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.
Maybe a better change could be
pub fn is_kde_session() -> bool {
let Ok(entries) = fs::read_dir("/proc") else {
return false;
};
for entry in entries.flatten() {
let name = entry.file_name();
let name_str = name.to_string_lossy();
// Skip non-pid directories
if !name_str.chars().all(|c| c.is_ascii_digit()) {
continue;
}
// Read /proc/[pid]/comm (process name, max 16 chars, no args)
let comm_path = entry.path().join("comm");
if let Ok(comm) = fs::read_to_string(&comm_path) {
let comm = comm.trim();
// Match kded5, kded6, etc.
if comm.starts_with("kded") && comm[4..].chars().all(|c| c.is_ascii_digit()) {
return true;
}
}
}
false
}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.
Indeed that’s much better, I’ll update that PR with your code. Using shell commands for pretty much anything is a red flag anyway, silencing the audit logs was always a bad workaround.
Someone on lobste.rs found those out two years ago, but besides complaining nobody fixed them… So here I am, I’ve only fixed the most glaring issues, I plan on replacing the remaining loginctl calls with zbus and then this issue can be closed as fixed.