Skip to content

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

Merged
Nox-MSFT merged 1 commit into
developfrom
user/nox-msft/issue-766
Jun 24, 2026
Merged

Fix logical bugs in adu-shell script execution (issue #766)#895
Nox-MSFT merged 1 commit into
developfrom
user/nox-msft/issue-766

Conversation

@Nox-MSFT

Copy link
Copy Markdown
Contributor

Summary

Fixes the logical bugs in adu-shell's Script::Execute() reported in #766, additionally restores file ownership after execution, and surfaces the resulting failures to IoT Hub as meaningful extended result codes.

Bug fixes (src/adu-shell/src/script_tasks.cpp)

Issue #766 identified three logical mistakes in Execute():

  1. A missing file was still executed. A failed stat() only skipped the permission/ownership block, but the child process was launched unconditionally afterward. It now logs a "file not found" error and returns the new ADUSHELL_EXIT_FILE_NOT_FOUND without executing.
  2. filePermissionsChanged was set in the wrong branch. It was set to true only when chmod failed, so successfully-changed permissions were never restored. It is now set after a successful chmod.
  3. The "restore" re-applied the new permissions. Cleanup called chmod(path, mode) with the new exec mode rather than the original. The original mode (captured up front) is now restored.

In addition, the original ownership is now restored after execution (previously the code permanently changed ownership and never restored it). Ownership is restored before permissions, because chown() clears the set-user-ID/set-group-ID bits.

Meaningful extended result codes

Previously any non-zero adu-shell exit code was reported to IoT Hub through the generic ADUC_ERC_SCRIPT_HANDLER_CHILD_PROCESS_FAILURE_EXITCODE(exitCode) wrapper, so e.g. a missing script surfaced as an opaque 0x3050100x value.

The script step handler now maps adu-shell's reserved sentinel exit codes to dedicated, documented ERCs:

adu-shell exit code ExtendedResultCode Value
ADUSHELL_EXIT_FILE_NOT_FOUND (6) ADUC_ERC_SCRIPT_HANDLER_EXECUTE_PRIMARY_FILE_NOT_FOUND 0x3050044C
ADUSHELL_EXIT_BAD_FILE_PERMS (4) ADUC_ERC_SCRIPT_HANDLER_EXECUTE_SET_PERMISSIONS_FAILURE 0x3050044D
ADUSHELL_EXIT_BAD_FILE_OWNERSHIP (5) ADUC_ERC_SCRIPT_HANDLER_EXECUTE_SET_OWNERSHIP_FAILURE 0x3050044E
ADUSHELL_EXIT_UNSUPPORTED (3) ADUC_ERC_SCRIPT_HANDLER_EXECUTE_UNSUPPORTED_ACTION 0x3050044F

A script's own (non-sentinel) exit code still uses the generic child-process wrapper. The new codes are defined in scripts/error_code_generator_defs/result_codes.json (the source of truth) and the generated src/inc/aduc/result.h.

Tests

Added Catch2 unit tests in src/adu-shell/tests/adushell_ut.cpp covering:

  • a missing script file is not executed and reports a failure exit status;
  • the original file permissions are restored after execution;
  • ownership is left unchanged when the configured user/group cannot be resolved (the path the unit-test build actually exercises).

The new tests and the pre-existing script-execute test now write to a unique per-run scratch directory under the build's configurable work folder (ADU_SHELL_TEST_TMP_DIR, derived from ADUC_TMP_DIR_PATH / --work-folder) with RAII cleanup, instead of a hardcoded /tmp path that may be unavailable.

Notes

Fixes #766

In adu-shell Script::Execute():

- Do not execute a script that does not exist; log a FileNotFound error and return the new ADUSHELL_EXIT_FILE_NOT_FOUND instead of launching a missing file.

- Set filePermissionsChanged only after a successful chmod (it was previously set only on chmod failure, so changed permissions were never restored).

- Restore the original permissions (not the new exec mode) after execution, and restore the original ownership as well (before permissions, since chown clears setuid/setgid bits).

Translate adu-shell's reserved exit codes into meaningful extended result codes in the script step handler (new ADUC_ERC_SCRIPT_HANDLER_EXECUTE_* codes), keeping the generic child-process wrapper for a script's own exit codes.

Add Catch2 unit tests for the missing-file, permission-restore, and ownership-unchanged behaviors. Tests use a unique scratch directory under the build's configurable work folder (ADU_SHELL_TEST_TMP_DIR derived from ADUC_TMP_DIR_PATH) with RAII cleanup instead of a hardcoded /tmp path.
@Nox-MSFT
Nox-MSFT requested review from a team and chgennar June 23, 2026 21:28
@Nox-MSFT
Nox-MSFT merged commit f291664 into develop Jun 24, 2026
12 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Logical bugs when executing scripts

2 participants