fix: improve hook command execution and parsing#1689
Conversation
1. Change hook command execution from shell `-c` argument to stdin pipe to avoid shell quoting and injection issues 2. Add `extractValue` lambda that trims whitespace and strips surrounding quotes (single or double) from extracted hook command values 3. Apply the new extraction logic to pre-install, post-install, and post-uninstall hook command parsing 4. Include `linglong/common/strings.h` for the `trim` utility function Influence: 1. Test pre-install hooks with double-quoted command values 2. Test post-install hooks with single-quoted command values 3. Test post-uninstall hooks with whitespace-padded command values 4. Verify hook commands with special characters execute correctly via stdin 5. Test hook execution with unquoted command values to ensure backward compatibility 6. Verify that commands containing quotes or shell metacharacters are handled safely
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: dengbo11 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 |
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request modifies hook command execution to pass commands via standard input instead of command-line arguments, and introduces an extractValue helper function to parse and trim hook commands from configuration files. The review feedback recommends improving extractValue to trim whitespace inside surrounding quotes and filtering out empty commands to avoid spawning unnecessary shell processes.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
| auto extractValue = [](const std::string &line, std::size_t prefixLen) -> std::string { | ||
| auto trimmed = linglong::common::strings::trim(std::string_view(line).substr(prefixLen)); | ||
| if (trimmed.size() >= 2 | ||
| && ((trimmed.front() == '"' && trimmed.back() == '"') | ||
| || (trimmed.front() == '\'' && trimmed.back() == '\''))) { | ||
| trimmed.remove_prefix(1); | ||
| trimmed.remove_suffix(1); | ||
| } | ||
| return std::string(trimmed); | ||
| }; |
There was a problem hiding this comment.
If the hook command contains leading or trailing whitespace inside the surrounding quotes (e.g., ll-pre-install=" echo hello "), stripping the quotes will leave the leading/trailing whitespace intact. Additionally, if the command only contains spaces inside the quotes (e.g., ll-pre-install=" "), it will result in a whitespace-only command. Trimming the string view again after stripping the quotes ensures that any such whitespace is cleaned up and empty commands are correctly identified.
| auto extractValue = [](const std::string &line, std::size_t prefixLen) -> std::string { | |
| auto trimmed = linglong::common::strings::trim(std::string_view(line).substr(prefixLen)); | |
| if (trimmed.size() >= 2 | |
| && ((trimmed.front() == '"' && trimmed.back() == '"') | |
| || (trimmed.front() == '\'' && trimmed.back() == '\''))) { | |
| trimmed.remove_prefix(1); | |
| trimmed.remove_suffix(1); | |
| } | |
| return std::string(trimmed); | |
| }; | |
| auto extractValue = [](const std::string &line, std::size_t prefixLen) -> std::string { | |
| auto trimmed = linglong::common::strings::trim(std::string_view(line).substr(prefixLen)); | |
| if (trimmed.size() >= 2 | |
| && ((trimmed.front() == '"' && trimmed.back() == '"') | |
| || (trimmed.front() == '\'' && trimmed.back() == '\''))) { | |
| trimmed.remove_prefix(1); | |
| trimmed.remove_suffix(1); | |
| trimmed = linglong::common::strings::trim(trimmed); | |
| } | |
| return std::string(trimmed); | |
| }; |
| std::size_t pos = line.find(PRE_INSTALL_ACTION_PREFIX); | ||
| if (pos != std::string::npos) { | ||
| preInstallCommands.emplace_back( | ||
| line.substr(pos + PRE_INSTALL_ACTION_PREFIX.length())); | ||
| extractValue(line, pos + PRE_INSTALL_ACTION_PREFIX.length())); | ||
| break; | ||
| } | ||
|
|
||
| pos = line.find(POST_INSTALL_ACTION_PREFIX); | ||
| if (pos != std::string::npos) { | ||
| postInstallCommands.emplace_back( | ||
| line.substr(pos + POST_INSTALL_ACTION_PREFIX.length())); | ||
| extractValue(line, pos + POST_INSTALL_ACTION_PREFIX.length())); | ||
| break; | ||
| } | ||
|
|
||
| pos = line.find(POST_UNINSTALL_ACTION_PREFIX); | ||
| if (pos != std::string::npos) { | ||
| postUninstallCommands.emplace_back( | ||
| line.substr(pos + POST_UNINSTALL_ACTION_PREFIX.length())); | ||
| extractValue(line, pos + POST_UNINSTALL_ACTION_PREFIX.length())); | ||
| break; | ||
| } |
There was a problem hiding this comment.
If a hook command is empty (e.g., ll-pre-install="" or ll-pre-install=), it is still added to the commands list. This causes executeInstallHooks to perform unnecessary operations (such as reading proc fd links) and ultimately spawns a shell process (sh) just to execute an empty command. Filtering out empty commands during parsing avoids this overhead entirely.
std::size_t pos = line.find(PRE_INSTALL_ACTION_PREFIX);
if (pos != std::string::npos) {
if (auto val = extractValue(line, pos + PRE_INSTALL_ACTION_PREFIX.length()); !val.empty()) {
preInstallCommands.emplace_back(std::move(val));
}
break;
}
pos = line.find(POST_INSTALL_ACTION_PREFIX);
if (pos != std::string::npos) {
if (auto val = extractValue(line, pos + POST_INSTALL_ACTION_PREFIX.length()); !val.empty()) {
postInstallCommands.emplace_back(std::move(val));
}
break;
}
pos = line.find(POST_UNINSTALL_ACTION_PREFIX);
if (pos != std::string::npos) {
if (auto val = extractValue(line, pos + POST_UNINSTALL_ACTION_PREFIX.length()); !val.empty()) {
postUninstallCommands.emplace_back(std::move(val));
}
break;
}
deepin pr auto review★ 总体评分：100分■ 【总体评价】
■ 【详细分析】
■ 【改进建议代码示例】 --- a/libs/utils/src/linglong/utils/hooks.cpp
+++ b/libs/utils/src/linglong/utils/hooks.cpp
@@ -47,6 +47,10 @@ utils::error::Result<void> executeHookCommands(
cmd.toStdin(command);
+ // 增加防御性错误处理：拦截空命令，防止无意义的子进程创建开销
+ if (command.empty()) {
+ return LINGLONG_ERR("Hook command is empty, skipping execution.");
+ }
auto result = cmd.exec({});
if (!result.has_value()) {
return LINGLONG_ERR( |
Codecov Report❌ Patch coverage is
🚀 New features to boost your workflow:
|
2 participants
-cargument to stdin pipe to avoid shell quoting and injection issuesextractValuelambda that trims whitespace and strips surrounding quotes (single or double) from extracted hook command valueslinglong/common/strings.hfor thetrimutility functionInfluence: