Fix logical bugs in adu-shell script execution (issue #766)

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
                         }
                     ]
                 },

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.

src/adu-shell/src/script_tasks.cpp

@@ -10,6 +10,8 @@
 #include "aduc/process_utils.hpp"
 #include "common_tasks.hpp"
 
+#include <cerrno>
+#include <cstring> // strerror
 #include <unordered_map>
 
 #include <aducpal/sys_stat.h> // 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);
         }
     }
 

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_FILE:adu-shell>")
 
 target_link_libraries (${PROJECT_NAME} PRIVATE Catch2::Catch2WithMain Catch2::Catch2 aduc::string_utils)

src/adu-shell/tests/adushell_ut.cpp

@@ -13,6 +13,8 @@
 
 #include <catch2/catch_all.hpp>
 
+#include <atomic>
+#include <chrono>
 #include <filesystem>
 #include <fstream>
 #include <string>
@@ -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<unsigned long long> counter{ 0 };
+        const auto nonce =
+            static_cast<unsigned long long>(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<std::string> 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<char*>(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<char*>(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<char*>(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")

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 <sstream>
 #include <string>
@@ -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;

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)
  */