diff --git a/scripts/error_code_generator_defs/result_codes.json b/scripts/error_code_generator_defs/result_codes.json index f1303e387..686fce7dc 100644 --- a/scripts/error_code_generator_defs/result_codes.json +++ b/scripts/error_code_generator_defs/result_codes.json @@ -494,6 +494,22 @@ { "name": "ADUC_ERC_SCRIPT_HANDLER_APPLY_FAILURE_UNKNOWNEXCEPTION", "value": 1023 + }, + { + "name": "ADUC_ERC_SCRIPT_HANDLER_EXECUTE_PRIMARY_FILE_NOT_FOUND", + "value": 1100 + }, + { + "name": "ADUC_ERC_SCRIPT_HANDLER_EXECUTE_SET_PERMISSIONS_FAILURE", + "value": 1101 + }, + { + "name": "ADUC_ERC_SCRIPT_HANDLER_EXECUTE_SET_OWNERSHIP_FAILURE", + "value": 1102 + }, + { + "name": "ADUC_ERC_SCRIPT_HANDLER_EXECUTE_UNSUPPORTED_ACTION", + "value": 1103 } ] }, diff --git a/src/adu-shell/inc/adushell.hpp b/src/adu-shell/inc/adushell.hpp index 7f4743733..3936d9e47 100644 --- a/src/adu-shell/inc/adushell.hpp +++ b/src/adu-shell/inc/adushell.hpp @@ -39,6 +39,7 @@ typedef struct tagADUShell_LaunchArguments #define ADUSHELL_EXIT_UNSUPPORTED 3 #define ADUSHELL_EXIT_BAD_FILE_PERMS 4 #define ADUSHELL_EXIT_BAD_FILE_OWNERSHIP 5 +#define ADUSHELL_EXIT_FILE_NOT_FOUND 6 /** * @brief An exit code for early exit of adu-shell process due to SIGTERM/SIGINT signal handling. diff --git a/src/adu-shell/src/script_tasks.cpp b/src/adu-shell/src/script_tasks.cpp index 679d941d9..adbd4446e 100644 --- a/src/adu-shell/src/script_tasks.cpp +++ b/src/adu-shell/src/script_tasks.cpp @@ -10,6 +10,8 @@ #include "aduc/process_utils.hpp" #include "common_tasks.hpp" +#include +#include // strerror #include #include // chmod @@ -47,15 +49,33 @@ ADUShellTaskResult Execute(const ADUShell_LaunchArguments& launchArgs) const char* path = launchArgs.targetData; struct stat st = {}; bool filePermissionsChanged = false; - bool statOk = stat(path, &st) == 0; + bool fileOwnershipChanged = false; int mode = S_IRWXU | S_IRGRP | S_IXGRP; - if (statOk) + int originalMode = 0; + uid_t originalUid = 0; + gid_t originalGid = 0; + + // Do not attempt to execute a script that does not exist. Otherwise the + // child process launch below would fail with a hard-to-diagnose error. + if (stat(path, &st) != 0) { - // Ensure that the script has the correct ownership. - struct group* grp = ADUCPAL_getgrnam(ADUC_FILE_GROUP); - struct passwd* p = ADUCPAL_getpwnam(ADUC_FILE_USER); + Log_Error("Cannot execute script '%s'. File not found (errno: %d, %s)", path, errno, strerror(errno)); + taskResult.SetExitStatus(ADUSHELL_EXIT_FILE_NOT_FOUND); + return taskResult; + } + + // Remember the original permissions and ownership so they can be restored after execution. + originalMode = st.st_mode & ~S_IFMT; + originalUid = st.st_uid; + originalGid = st.st_gid; - if (p != NULL && grp != NULL) + // Ensure that the script has the correct ownership. + struct group* grp = ADUCPAL_getgrnam(ADUC_FILE_GROUP); + struct passwd* p = ADUCPAL_getpwnam(ADUC_FILE_USER); + + if (p != NULL && grp != NULL) + { + if (originalUid != p->pw_uid || originalGid != grp->gr_gid) { // Fix the ownership. if (0 != ADUCPAL_chown(path, p->pw_uid, grp->gr_gid)) @@ -64,35 +84,48 @@ ADUShellTaskResult Execute(const ADUShell_LaunchArguments& launchArgs) taskResult.SetExitStatus(ADUSHELL_EXIT_BAD_FILE_OWNERSHIP); goto done; } + + // The ownership was successfully changed and must be restored after execution. + fileOwnershipChanged = true; } + } - int perms = st.st_mode & ~S_IFMT; - if (perms != mode) + if (originalMode != mode) + { + // Fix the permissions. + if (0 != ADUCPAL_chmod(path, mode)) { - // Fix the permissions. - if (0 != ADUCPAL_chmod(path, mode)) - { - filePermissionsChanged = true; - stat(path, &st); - Log_Error( - "Failed to set '%s' file permissions (expected:%d, actual: %d)", path, mode, st.st_mode & ~S_IFMT); - taskResult.SetExitStatus(ADUSHELL_EXIT_BAD_FILE_PERMS); - goto done; - } + stat(path, &st); + Log_Error( + "Failed to set '%s' file permissions (expected:%d, actual: %d)", path, mode, st.st_mode & ~S_IFMT); + taskResult.SetExitStatus(ADUSHELL_EXIT_BAD_FILE_PERMS); + goto done; } + + // The permissions were successfully changed and must be restored after execution. + filePermissionsChanged = true; } taskResult.SetExitStatus(ADUC_LaunchChildProcess(launchArgs.targetData, args, taskResult.Output())); done: - // Restore the permissions. + // Restore the original ownership if it was changed. This is done before + // restoring the permissions because chown() may clear the set-user-ID and + // set-group-ID permission bits. + if (fileOwnershipChanged) + { + if (0 != ADUCPAL_chown(path, originalUid, originalGid)) + { + Log_Warn("Failed to restore '%s' file ownership", path); + } + } + + // Restore the original permissions if they were changed. if (filePermissionsChanged) { - // Restore the permissions. - if (0 != ADUCPAL_chmod(path, mode)) + if (0 != ADUCPAL_chmod(path, originalMode)) { - stat(path, &st); - Log_Warn("Failed restore '%s' file permissions", path); + Log_Warn("Failed to restore '%s' file permissions", path); } } diff --git a/src/adu-shell/tests/CMakeLists.txt b/src/adu-shell/tests/CMakeLists.txt index 2ce67dbc7..6dabb3d6b 100644 --- a/src/adu-shell/tests/CMakeLists.txt +++ b/src/adu-shell/tests/CMakeLists.txt @@ -31,6 +31,7 @@ target_compile_definitions ( PRIVATE ADUC_USE_XLOGGING=1 ADUC_FILE_USER="__adu_shell_test_user__" ADUC_FILE_GROUP="__adu_shell_test_group__" + ADU_SHELL_TEST_TMP_DIR="${ADUC_TMP_DIR_PATH}/adu/adushell-unit-tests" ADU_SHELL_BIN_PATH="$") target_link_libraries (${PROJECT_NAME} PRIVATE Catch2::Catch2WithMain Catch2::Catch2 aduc::string_utils) diff --git a/src/adu-shell/tests/adushell_ut.cpp b/src/adu-shell/tests/adushell_ut.cpp index fa7e0340e..292f84dab 100644 --- a/src/adu-shell/tests/adushell_ut.cpp +++ b/src/adu-shell/tests/adushell_ut.cpp @@ -13,6 +13,8 @@ #include +#include +#include #include #include #include @@ -38,6 +40,55 @@ struct LaunchCapture }; LaunchCapture g_launchCapture; + +// Tests in this file create, chmod/chown, and delete script files at runtime. +// They must write to a writable scratch directory under the build's configurable +// work folder (ADU_SHELL_TEST_TMP_DIR is derived from ADUC_TMP_DIR_PATH / the +// --work-folder), rather than the system temp directory (which may be +// unavailable) or the read-only test-data fixtures. Each ScopedTempDir is a +// unique per-run directory that is removed on destruction, keeping the tests +// hermetic and parallel-safe. +#ifndef ADU_SHELL_TEST_TMP_DIR +# define ADU_SHELL_TEST_TMP_DIR "/tmp/adu/adushell-unit-tests" +#endif + +class ScopedTempDir +{ +public: + ScopedTempDir() + { + static std::atomic counter{ 0 }; + const auto nonce = + static_cast(std::chrono::steady_clock::now().time_since_epoch().count()); + _path = std::filesystem::path(ADU_SHELL_TEST_TMP_DIR) + / ("adushell-ut-" + std::to_string(nonce) + "-" + std::to_string(counter.fetch_add(1))); + + std::error_code ec; + std::filesystem::create_directories(_path, ec); + } + + ~ScopedTempDir() + { + std::error_code ec; + std::filesystem::remove_all(_path, ec); + } + + ScopedTempDir(const ScopedTempDir&) = delete; + ScopedTempDir& operator=(const ScopedTempDir&) = delete; + + std::filesystem::path file(const char* name) const + { + return _path / name; + } + + const std::filesystem::path& path() const + { + return _path; + } + +private: + std::filesystem::path _path; +}; } int ADUC_LaunchChildProcess(const std::string& command, std::vector args, std::string& output) @@ -208,7 +259,8 @@ TEST_CASE("Script execute runs script with target options") { g_launchCapture.Reset(); - const auto scriptPath = std::filesystem::temp_directory_path() / "adushell_script_task_test.sh"; + ScopedTempDir tmp; + const auto scriptPath = tmp.file("script.sh"); { std::ofstream script(scriptPath); REQUIRE(script.good()); @@ -230,9 +282,94 @@ TEST_CASE("Script execute runs script with target options") REQUIRE(g_launchCapture.args.size() == 2); CHECK(g_launchCapture.args[0] == "--first"); CHECK(g_launchCapture.args[1] == "--second"); +} + +TEST_CASE("Script execute does not run a script that does not exist") +{ + g_launchCapture.Reset(); + g_launchCapture.command = "__NOT_CALLED__"; + + ScopedTempDir tmp; + const auto missingPath = tmp.file("missing.sh"); + REQUIRE_FALSE(std::filesystem::exists(missingPath)); + const std::string missingStr = missingPath.string(); + + ADUShell_LaunchArguments launchArgs{}; + launchArgs.targetData = const_cast(missingStr.c_str()); + + auto taskResult = Adu::Shell::Tasks::Script::Execute(launchArgs); + + // The child process must NOT be launched when the script file is missing (issue #766). + CHECK(g_launchCapture.command == "__NOT_CALLED__"); + // And the task must report a failure exit status. + CHECK(taskResult.ExitStatus() != EXIT_SUCCESS); +} + +TEST_CASE("Script execute restores original file permissions") +{ + g_launchCapture.Reset(); + + ScopedTempDir tmp; + const auto scriptPath = tmp.file("script.sh"); + { + std::ofstream script(scriptPath); + REQUIRE(script.good()); + script << "#!/bin/sh\n"; + script << "echo ok\n"; + } + + // Original permissions intentionally differ from the 0750 exec mode adu-shell applies. + const mode_t originalMode = S_IRUSR | S_IWUSR; // 0600 + REQUIRE(::chmod(scriptPath.c_str(), originalMode) == 0); + + const std::string scriptStr = scriptPath.string(); + ADUShell_LaunchArguments launchArgs{}; + launchArgs.targetData = const_cast(scriptStr.c_str()); + + auto taskResult = Adu::Shell::Tasks::Script::Execute(launchArgs); + CHECK(taskResult.ExitStatus() == EXIT_SUCCESS); + + struct stat st = {}; + REQUIRE(::stat(scriptPath.c_str(), &st) == 0); + // After execution the original permissions must be restored (issue #766). + CHECK((st.st_mode & 07777) == originalMode); +} + +TEST_CASE("Script execute leaves ownership unchanged when the configured user/group cannot be resolved") +{ + // The unit-test build configures ADUC_FILE_USER/ADUC_FILE_GROUP with names + // that do not exist on the system, so the ownership-adjustment branch is + // skipped and the file's ownership must be left untouched. (Exercising the + // full chown-and-restore path requires a resolvable target user and root.) + g_launchCapture.Reset(); + + ScopedTempDir tmp; + const auto scriptPath = tmp.file("script.sh"); + { + std::ofstream script(scriptPath); + REQUIRE(script.good()); + script << "#!/bin/sh\n"; + script << "echo ok\n"; + } + + // Use the exact exec mode so the permission path is a no-op and the test + // isolates the ownership behavior. + REQUIRE(::chmod(scriptPath.c_str(), S_IRWXU | S_IRGRP | S_IXGRP) == 0); + + struct stat before = {}; + REQUIRE(::stat(scriptPath.c_str(), &before) == 0); + + const std::string scriptStr = scriptPath.string(); + ADUShell_LaunchArguments launchArgs{}; + launchArgs.targetData = const_cast(scriptStr.c_str()); + + auto taskResult = Adu::Shell::Tasks::Script::Execute(launchArgs); + CHECK(taskResult.ExitStatus() == EXIT_SUCCESS); - std::error_code err; - std::filesystem::remove(scriptPath, err); + struct stat after = {}; + REQUIRE(::stat(scriptPath.c_str(), &after) == 0); + CHECK(after.st_uid == before.st_uid); + CHECK(after.st_gid == before.st_gid); } TEST_CASE("Script do-task handles unsupported action") diff --git a/src/extensions/step_handlers/script_handler/src/script_handler.cpp b/src/extensions/step_handlers/script_handler/src/script_handler.cpp index 49011d498..5da86eb10 100644 --- a/src/extensions/step_handlers/script_handler/src/script_handler.cpp +++ b/src/extensions/step_handlers/script_handler/src/script_handler.cpp @@ -17,6 +17,7 @@ #include "aduc/types/adu_core.h" // ADUC_Result_* #include "aduc/workflow_data_utils.h" // ADUC_WorkflowData_GetWorkFolder #include "aduc/workflow_utils.h" // workflow_* +#include "adushell.hpp" // ADUSHELL_EXIT_* exit codes #include "adushell_const.hpp" #include #include @@ -665,7 +666,29 @@ ScriptHandler_PerformAction(const std::string& action, const tagADUC_WorkflowDat if (exitCode != 0) { - int extendedCode = ADUC_ERC_SCRIPT_HANDLER_CHILD_PROCESS_FAILURE_EXITCODE(exitCode); + // Translate adu-shell's reserved exit codes (see adushell.hpp) into meaningful + // extended result codes. Any other value is treated as the script's own exit + // code and reported with the generic child-process wrapper. + int extendedCode; + switch (exitCode) + { + case ADUSHELL_EXIT_FILE_NOT_FOUND: + extendedCode = ADUC_ERC_SCRIPT_HANDLER_EXECUTE_PRIMARY_FILE_NOT_FOUND; + break; + case ADUSHELL_EXIT_BAD_FILE_PERMS: + extendedCode = ADUC_ERC_SCRIPT_HANDLER_EXECUTE_SET_PERMISSIONS_FAILURE; + break; + case ADUSHELL_EXIT_BAD_FILE_OWNERSHIP: + extendedCode = ADUC_ERC_SCRIPT_HANDLER_EXECUTE_SET_OWNERSHIP_FAILURE; + break; + case ADUSHELL_EXIT_UNSUPPORTED: + extendedCode = ADUC_ERC_SCRIPT_HANDLER_EXECUTE_UNSUPPORTED_ACTION; + break; + default: + extendedCode = ADUC_ERC_SCRIPT_HANDLER_CHILD_PROCESS_FAILURE_EXITCODE(exitCode); + break; + } + Log_Error("Script failed (%s), extendedResultCode:0x%X (exitCode:%d)", action.c_str(), extendedCode, exitCode); results.result.ResultCode = ADUC_Result_Failure; results.result.ExtendedResultCode = extendedCode; diff --git a/src/inc/aduc/result.h b/src/inc/aduc/result.h index b46f93b3e..cebbd021c 100644 --- a/src/inc/aduc/result.h +++ b/src/inc/aduc/result.h @@ -1060,6 +1060,26 @@ static inline ADUC_Result_t MAKE_ADUC_EXTENDEDRESULTCODE_FOR_FACILITY_ADUC_FACIL */ #define ADUC_ERC_SCRIPT_HANDLER_APPLY_FAILURE_UNKNOWNEXCEPTION MAKE_ADUC_EXTENDEDRESULTCODE_FOR_COMPONENT_ADUC_CONTENT_HANDLER_SCRIPT(1023) +/** + * @brief ADUC_ERC_SCRIPT_HANDLER_EXECUTE_PRIMARY_FILE_NOT_FOUND, ERC Value: 810550348 (0x3050044c) + */ + #define ADUC_ERC_SCRIPT_HANDLER_EXECUTE_PRIMARY_FILE_NOT_FOUND MAKE_ADUC_EXTENDEDRESULTCODE_FOR_COMPONENT_ADUC_CONTENT_HANDLER_SCRIPT(1100) + +/** + * @brief ADUC_ERC_SCRIPT_HANDLER_EXECUTE_SET_PERMISSIONS_FAILURE, ERC Value: 810550349 (0x3050044d) + */ + #define ADUC_ERC_SCRIPT_HANDLER_EXECUTE_SET_PERMISSIONS_FAILURE MAKE_ADUC_EXTENDEDRESULTCODE_FOR_COMPONENT_ADUC_CONTENT_HANDLER_SCRIPT(1101) + +/** + * @brief ADUC_ERC_SCRIPT_HANDLER_EXECUTE_SET_OWNERSHIP_FAILURE, ERC Value: 810550350 (0x3050044e) + */ + #define ADUC_ERC_SCRIPT_HANDLER_EXECUTE_SET_OWNERSHIP_FAILURE MAKE_ADUC_EXTENDEDRESULTCODE_FOR_COMPONENT_ADUC_CONTENT_HANDLER_SCRIPT(1102) + +/** + * @brief ADUC_ERC_SCRIPT_HANDLER_EXECUTE_UNSUPPORTED_ACTION, ERC Value: 810550351 (0x3050044f) + */ + #define ADUC_ERC_SCRIPT_HANDLER_EXECUTE_UNSUPPORTED_ACTION MAKE_ADUC_EXTENDEDRESULTCODE_FOR_COMPONENT_ADUC_CONTENT_HANDLER_SCRIPT(1103) + /** * @brief ADUC_ERC_CONTENT_DOWNLOADER_CREATE_FAILURE_NO_SYMBOL, ERC Value: 1073741825 (0x40000001) */