Skip to content

Revert "refactor: use Cmd class for hook command execution" #1690

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.

Sign up for GitHub

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

Closed
dengbo11 wants to merge 1 commit into from
Closed

Revert "refactor: use Cmd class for hook command execution" #1690

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
57 changes: 46 additions & 11 deletions libs/utils/src/linglong/utils/hooks.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,24 +1,25 @@
/*
* SPDX-FileCopyrightText: 2025 - 2026 UnionTech Software Technology Co., Ltd.
* SPDX-FileCopyrightText: 2025 UnionTech Software Technology Co., Ltd.
*
* SPDX-License-Identifier: LGPL-3.0-or-later
*/

#include "hooks.h"

#include "cmd.h"
#include "configure.h"
#include "linglong/common/error.h"

Check warning on line 10 in libs/utils/src/linglong/utils/hooks.cpp

View workflow job for this annotation

GitHub Actions / cppcheck 

Include file: "configure.h" not found.
#include "linglong/utils/env.h"

Check warning on line 11 in libs/utils/src/linglong/utils/hooks.cpp

View workflow job for this annotation

GitHub Actions / cppcheck 

Include file: "linglong/common/error.h" not found.
#include "linglong/utils/error/error.h"

Check warning on line 12 in libs/utils/src/linglong/utils/hooks.cpp

View workflow job for this annotation

GitHub Actions / cppcheck 

Include file: "linglong/utils/error/error.h" not found.
#include "linglong/utils/log/log.h"

Check warning on line 13 in libs/utils/src/linglong/utils/hooks.cpp

View workflow job for this annotation

GitHub Actions / cppcheck 

Include file: "linglong/utils/log/log.h" not found.

#include <fmt/format.h>

#include <array>

Check warning on line 17 in libs/utils/src/linglong/utils/hooks.cpp

View workflow job for this annotation

GitHub Actions / cppcheck 

Include file: <array> not found. Please note: Cppcheck does not need standard library headers to get proper results.
#include <climits>

Check warning on line 18 in libs/utils/src/linglong/utils/hooks.cpp

View workflow job for this annotation

GitHub Actions / cppcheck 

Include file: <climits> not found. Please note: Cppcheck does not need standard library headers to get proper results.
#include <cstdlib>

Check warning on line 19 in libs/utils/src/linglong/utils/hooks.cpp

View workflow job for this annotation

GitHub Actions / cppcheck 

Include file: <cstring> not found. Please note: Cppcheck does not need standard library headers to get proper results.
#include <cstring>

Check warning on line 20 in libs/utils/src/linglong/utils/hooks.cpp

View workflow job for this annotation

GitHub Actions / cppcheck 

Include file: <filesystem> not found. Please note: Cppcheck does not need standard library headers to get proper results.
#include <filesystem>

Check warning on line 21 in libs/utils/src/linglong/utils/hooks.cpp

View workflow job for this annotation

GitHub Actions / cppcheck 

Include file: <fstream> not found. Please note: Cppcheck does not need standard library headers to get proper results.
#include <fstream>

Check warning on line 22 in libs/utils/src/linglong/utils/hooks.cpp

View workflow job for this annotation

GitHub Actions / cppcheck 

Include file: <sstream> not found. Please note: Cppcheck does not need standard library headers to get proper results.
#include <sstream>
#include <vector>

Expand All @@ -32,24 +33,58 @@
constexpr std::string_view POST_INSTALL_ACTION_PREFIX = "ll-post-install=";
constexpr std::string_view POST_UNINSTALL_ACTION_PREFIX = "ll-post-uninstall=";

// This function ensures the command string is safely wrapped for 'sh -c'.
static std::string escapeAndWrapCommandForShell(const std::string &command)
{
std::string escapedCommand = command;
size_t pos = 0;
// Replace ' with '\'' (close current quote, add literal single quote, open new quote)
while ((pos = escapedCommand.find('\'', pos)) != std::string::npos) {
escapedCommand.replace(pos, 1, "'\\''");
pos += 4;
}

return "sh -c " + escapedCommand;
}
Comment on lines +37 to +48

@gemini-code-assist gemini-code-assist Bot Jun 24, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

critical

The escapeAndWrapCommandForShell function is missing single quotes around the command string in its return statement. Currently, it returns "sh -c " + escapedCommand.

Without single quotes, a command with arguments (e.g., echo foo bar) is passed to std::system as sh -c echo foo bar. The outer shell executes sh -c echo foo bar, which runs sh with -c and echo as the command string, treating foo and bar as positional parameters ($0, $1) for the inner shell. Since the command string echo does not reference these positional parameters, they are completely ignored, and the command runs without arguments (e.g., printing an empty line instead of foo bar). This completely breaks any hook command that contains arguments.

Recommended Solutions:

  1. Simplest & Best Approach: Completely remove escapeAndWrapCommandForShell and pass command_raw directly to std::system. Since std::system already executes the command string by invoking /bin/sh -c, wrapping it in another sh -c is redundant, spawns an extra shell process, and introduces unnecessary escaping complexity.

  2. Alternative (if explicit wrapping is required): Wrap the escaped command in single quotes as shown in the suggestion below.

Suggested change
static std::string escapeAndWrapCommandForShell(const std::string &command)
{
std::string escapedCommand = command;
size_t pos = 0;
// Replace ' with '\'' (close current quote, add literal single quote, open new quote)
while ((pos = escapedCommand.find('\'', pos)) != std::string::npos) {
escapedCommand.replace(pos, 1, "'\\''");
pos += 4;
}
return "sh -c " + escapedCommand;
}
static std::string escapeAndWrapCommandForShell(const std::string &command)
{
std::string escapedCommand = command;
size_t pos = 0;
// Replace ' with '\'' (close current quote, add literal single quote, open new quote)
while ((pos = escapedCommand.find('\'', pos)) != std::string::npos) {
escapedCommand.replace(pos, 1, "'\\''");
pos += 4;
}
return "sh -c '" + escapedCommand + "'";
}


using CommandList = const std::vector<std::string> &;

utils::error::Result<void> executeHookCommands(
CommandList commands, const std::vector<std::pair<std::string, std::string>> &envVars) noexcept
{
LINGLONG_TRACE("Executing hook commands");
LINGLONG_TRACE("Executing command");

for (const auto &command : commands) {
Cmd cmd("sh");
std::vector<std::unique_ptr<EnvironmentVariableGuard>> envVarGuards;
envVarGuards.reserve(envVars.size());
for (const auto &pair : envVars) {
envVarGuards.emplace_back(
std::make_unique<EnvironmentVariableGuard>(pair.first, pair.second));
}

for (const auto &[name, value] : envVars) {
cmd.setEnv(name, value);
for (const auto &command_raw : commands) {
std::string fullCommand = escapeAndWrapCommandForShell(command_raw);

int ret = std::system(fullCommand.c_str());

if (ret == -1) {
return LINGLONG_ERR(fmt::format("Failed to execute command: '{}'. System error: {}.",
fullCommand,
common::error::errorString(errno)));
}

auto result = cmd.exec({ "-c", command });
if (!result.has_value()) {
return LINGLONG_ERR(
fmt::format("Hook command '{}' failed: {}.", command, result.error()));
if (!WIFEXITED(ret)) {
int signalNum = WTERMSIG(ret);
return LINGLONG_ERR(fmt::format("Command '{}' terminated by signal {} ({}).",
fullCommand,
signalNum,
strsignal(signalNum)));
}

int exitStatus = WEXITSTATUS(ret);
if (exitStatus != 0) {
return LINGLONG_ERR(fmt::format("Command '{}' exited with non-zero status: {}.",
fullCommand,
exitStatus));
}
}
return LINGLONG_OK;
Expand Down
Loading