From a5b28ff9acce002e3aba575c0bafbd1561414b98 Mon Sep 17 00:00:00 2001 From: Nick Anderson Date: Wed, 29 Jul 2026 01:19:27 -0500 Subject: [PATCH] Fixed usemodule() reporting success when the module exited non-zero usemodule() ignored the module's exit status and defined its class either way. It returns false now when the module ran and failed, and only fails as a function when the module could not be run or read at all. The distinction matters: a failed function call leaves the class undefined rather than false, so "not => usemodule(...)" would not have become true either. The test could not pass or fail whatever the code did. It looked for the modules one directory too high, its Pass guard asked for a class and its own negation, and the module it runs exits 1 on purpose which failed the whole test bundle. With those corrected it fails on the old code and passes on the new. Ticket: CFE-942 Changelog: Title Co-Authored-By: Claude Opus 5 (1M context) --- libpromises/evalfunction.c | 18 ++++++++++++----- .../02_functions/usemodule-returns-nonzero.cf | 20 ++++++++++++------- 2 files changed, 26 insertions(+), 12 deletions(-) diff --git a/libpromises/evalfunction.c b/libpromises/evalfunction.c index 8dfcdc8e42..45a015341c 100644 --- a/libpromises/evalfunction.c +++ b/libpromises/evalfunction.c @@ -114,7 +114,7 @@ static char *StripPatterns(char *file_buffer, const char *pattern, const char *f static int BuildLineArray(EvalContext *ctx, const Bundle *bundle, const char *array_lval, const char *file_buffer, const char *split, int maxent, DataType type, bool int_index); static JsonElement* BuildData(EvalContext *ctx, const char *file_buffer, const char *split, int maxent, bool make_array); -static bool ExecModule(EvalContext *ctx, char *command); +static bool ExecModule(EvalContext *ctx, char *command, int *retcode); static bool CheckIDChar(const char ch); static bool CheckID(const char *id); @@ -3199,12 +3199,15 @@ static FnCallResult FnCallUseModule(EvalContext *ctx, Log(LOG_LEVEL_VERBOSE, "Executing and using module [%s]", modulecmd); - if (!ExecModule(ctx, modulecmd)) + /* A module which exits non-zero has not told us anything we should act on, + * so the function is false. Not being able to run it at all is a failure. */ + int retcode = 0; + if (!ExecModule(ctx, modulecmd, &retcode)) { return FnFailure(); } - return FnReturnContext(true); + return FnReturnContext(retcode == 0); } /*********************************************************************/ @@ -10083,7 +10086,7 @@ static FnCallResult FnCallFindfilesUp(ARG_UNUSED EvalContext *ctx, ARG_UNUSED co /*********************************************************************/ -static bool ExecModule(EvalContext *ctx, char *command) +static bool ExecModule(EvalContext *ctx, char *command, int *retcode) { FILE *pp = cf_popen(command, "rt", true); if (!pp) @@ -10114,7 +10117,7 @@ static bool ExecModule(EvalContext *ctx, char *command) ModuleProtocol(ctx, command, line, print, context, sizeof(context), tags, &persistence); } bool atend = feof(pp); - cf_pclose(pp); + *retcode = cf_pclose(pp); free(line); StringSetDestroy(tags); @@ -10124,6 +10127,11 @@ static bool ExecModule(EvalContext *ctx, char *command) return false; } + if (*retcode != 0) + { + Log(LOG_LEVEL_ERR, "Module '%s' returned non-zero exit code %d", command, *retcode); + } + return true; } diff --git a/tests/acceptance/02_classes/02_functions/usemodule-returns-nonzero.cf b/tests/acceptance/02_classes/02_functions/usemodule-returns-nonzero.cf index 5c0bfd2daf..9c54395dbb 100644 --- a/tests/acceptance/02_classes/02_functions/usemodule-returns-nonzero.cf +++ b/tests/acceptance/02_classes/02_functions/usemodule-returns-nonzero.cf @@ -43,37 +43,43 @@ bundle agent test "description" -> { "CFE-942" } string => "Test that when a module executed by usemodule() returns nonzero, it's not interpreted as successful"; - "test_soft_fail" - string => "any", - meta => { "CFE-942" }, - comment => "usemodule seems to return true no matter if the module exists returning 0 or nonzero"; + # The modules this test writes are /bin/sh scripts + "test_skip_unsupported" string => "windows"; classes: # Since the module exits with non zero, we should not get this class "usemodule_expect_no_class_defined_because_return_nonzero" expression => usemodule("foo-usemodule", ""), scope => "namespace", - if => isexecutable("$(sys.workdir)/foo-usemodule"); + if => isexecutable("$(sys.workdir)/modules/foo-usemodule"); # Since the module exists non zero, we should get this class "usemodule_expect_class_defined_because_return_nonzero" not => usemodule("foo-usemodule", ""), scope => "namespace", - if => isexecutable("$(sys.workdir)/foo-usemodule"); + if => isexecutable("$(sys.workdir)/modules/foo-usemodule"); commands: "$(sys.workdir)/modules/foo-commands_module" module => "true", + classes => expected_nonzero_exit, if => isexecutable($(this.promiser)); } +body classes expected_nonzero_exit +# @brief The module here exits non-zero on purpose, so don't fail the promise +# over it and take the whole test bundle down with it +{ + kept_returncodes => { "1" }; +} + bundle agent check { methods: usemodule_expect_no_class_defined_because_return_nonzero.!usemodule_expect_class_defined_because_return_nonzero:: "FAIL" usebundle => dcs_fail($(this.promise_filename)); - !usemodule_expect_class_defined_because_return_nonzero.usemodule_expect_class_defined_because_return_nonzero:: + !usemodule_expect_no_class_defined_because_return_nonzero.usemodule_expect_class_defined_because_return_nonzero:: "Pass" usebundle => dcs_pass($(this.promise_filename)); reports: