From 5ddb6d97b11ed82655887decd48cb39c221275c8 Mon Sep 17 00:00:00 2001 From: Daniel JB Clark Date: Sat, 22 Aug 2026 18:21:46 -0400 Subject: [PATCH] Fixed simulate mode reporting both removal and install of the same package In DiffPkgOperations() and ManifestPkgOperations(), inserting an install message into the installed/present map set name_arch to NULL because the map took ownership of it as a key. The MapRemove() call that cancels a previous removal message ran after that, so it looked up a NULL key and matched nothing. The cancellation was silently skipped exactly when an install message had been inserted: a package recorded as removed and then installed was reported as both removed and installed. In debug builds a NULL key also fails an assertion in StringHash() once the map outgrows the array-map stage. Moved the cancellation above the code that hands name_arch to the map. Changelog: Title Ticket: CFE-4742 Co-Authored-By: Claude Fable 5 --- cf-agent/simulate_mode.c | 12 ++-- tests/unit/Makefile.am | 3 + tests/unit/simulate_mode_test.c | 123 ++++++++++++++++++++++++++++++++ 3 files changed, 132 insertions(+), 6 deletions(-) create mode 100644 tests/unit/simulate_mode_test.c diff --git a/cf-agent/simulate_mode.c b/cf-agent/simulate_mode.c index 2512617d21e..fa038706628 100644 --- a/cf-agent/simulate_mode.c +++ b/cf-agent/simulate_mode.c @@ -823,6 +823,9 @@ bool DiffPkgOperations() * operation. So 'install' operation must mean a newer version than what's present would * be installed. */ + /* Package installation cancels a previous removal (if any). */ + MapRemove(removed, name_arch); + PkgOperationRecord *prev_record = MapGet(installed, name_arch); if ((prev_record == NULL) || PkgVersionIsGreater(pkg_ver, prev_record->pkg_ver)) { @@ -832,9 +835,6 @@ bool DiffPkgOperations() MapInsert(installed, name_arch, record); name_arch = NULL; /* name_arch is now owned by the map (as a key) */ } - - /* Package installation cancels a previous removal (if any). */ - MapRemove(removed, name_arch); } else { @@ -972,6 +972,9 @@ bool ManifestPkgOperations() { /* If there is a previous install/present operation, we want to choose the message with * the higher version or the message which has a specific version (if any). */ + /* Cancels any previous remove/absent message. */ + MapRemove(absent, name_arch); + PkgOperationRecord *prev_record = MapGet(present, name_arch); if ((prev_record == NULL) || (NULL_OR_EMPTY(prev_record->pkg_ver) && !NULL_OR_EMPTY(pkg_ver)) || @@ -984,9 +987,6 @@ bool ManifestPkgOperations() MapInsert(present, name_arch, record); name_arch = NULL; /* name_arch is now owned by the map (as a key) */ } - - /* Cancels any previous remove/absent message. */ - MapRemove(absent, name_arch); } else { diff --git a/tests/unit/Makefile.am b/tests/unit/Makefile.am index 024e25f78b7..696c34e29c2 100644 --- a/tests/unit/Makefile.am +++ b/tests/unit/Makefile.am @@ -138,6 +138,7 @@ check_PROGRAMS = \ variable_test \ verify_databases_test \ files_properties_test \ + simulate_mode_test \ protocol_test \ mon_cpu_test \ mon_load_test \ @@ -402,6 +403,8 @@ verify_databases_test_LDADD = ../../cf-agent/libcf-agent.la libtest.la files_properties_test_LDADD = ../../cf-agent/libcf-agent.la libtest.la +simulate_mode_test_LDADD = ../../cf-agent/libcf-agent.la libtest.la + iteration_test_SOURCES = iteration_test.c cf_upgrade_test_SOURCES = cf_upgrade_test.c \ diff --git a/tests/unit/simulate_mode_test.c b/tests/unit/simulate_mode_test.c new file mode 100644 index 00000000000..4e0862f858e --- /dev/null +++ b/tests/unit/simulate_mode_test.c @@ -0,0 +1,123 @@ +/* + Copyright 2024 Northern.tech AS + + This file is part of CFEngine 3 - written and maintained by Northern.tech AS. + + This program is free software; you can redistribute it and/or modify it + under the terms of the GNU General Public License as published by the + Free Software Foundation; version 3. + + This program is distributed in the hope that it will be useful, + but WITHOUT ANY WARRANTY; without even the implied warranty of + MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the + GNU General Public License for more details. + + You should have received a copy of the GNU General Public License + along with this program; if not, write to the Free Software + Foundation, Inc., 59 Temple Place - Suite 330, Boston, MA 02111-1307, USA + + To the extent this program is licensed as part of the Enterprise + versions of CFEngine, the applicable Commercial Open Source License + (COSL) may apply to this file if you as a licensee so wish it. See + included file COSL.txt. +*/ + +#include + +#include /* SetChangesChroot(), ToChangesChroot() */ +#include /* CHROOT_PKGS_OPS_FILE */ +#include /* DeleteDirectoryTree() */ + +#include + +static char CHROOT_DIR[] = "/tmp/simulate_mode_test_chroot.XXXXXX"; + +/* #csv contains "op,name,version,architecture" records terminated by "\r\n", + * just like the records written by RecordPkgOperationInChroot(). */ +static void write_pkgs_ops(const char *csv) +{ + FILE *file = fopen(ToChangesChroot(CHROOT_PKGS_OPS_FILE), "w"); + assert_true(file != NULL); + assert_true(fputs(csv, file) >= 0); + fclose(file); +} + +/* Both DiffPkgOperations() and ManifestPkgOperations() print their reports + * with puts(), so capture stdout into #output while calling #fn. */ +static bool call_with_captured_stdout(bool (*fn)(void), char *output, size_t output_size) +{ + fflush(stdout); + int saved_stdout = dup(STDOUT_FILENO); + assert_true(saved_stdout != -1); + + char out_file[] = "/tmp/simulate_mode_test_out.XXXXXX"; + int out_fd = mkstemp(out_file); + assert_true(out_fd != -1); + assert_true(dup2(out_fd, STDOUT_FILENO) != -1); + + const bool ret = fn(); + + fflush(stdout); + assert_true(dup2(saved_stdout, STDOUT_FILENO) != -1); + close(saved_stdout); + + assert_true(lseek(out_fd, 0, SEEK_SET) != -1); + const ssize_t n_read = read(out_fd, output, output_size - 1); + assert_true(n_read >= 0); + output[n_read] = '\0'; + close(out_fd); + unlink(out_file); + + return ret; +} + +/* A recorded removal followed by a recorded installation of the same package + * is a net installation, so the removal must not be reported. */ +static void test_diff_install_cancels_removal(void) +{ + write_pkgs_ops("r,foo,,\r\n" + "i,foo,1.2.3,\r\n"); + + char output[4096]; + assert_true(call_with_captured_stdout(&DiffPkgOperations, output, sizeof(output))); + + assert_true(strstr(output, "Package 'foo [1.2.3]' would be installed") != NULL); + assert_true(strstr(output, "would be removed") == NULL); + + unlink(ToChangesChroot(CHROOT_PKGS_OPS_FILE)); +} + +static void test_manifest_install_cancels_removal(void) +{ + write_pkgs_ops("r,foo,,\r\n" + "i,foo,1.2.3,\r\n"); + + char output[4096]; + assert_true(call_with_captured_stdout(&ManifestPkgOperations, output, sizeof(output))); + + assert_true(strstr(output, "Package 'foo [1.2.3]' would be present") != NULL); + assert_true(strstr(output, "would be absent") == NULL); + + unlink(ToChangesChroot(CHROOT_PKGS_OPS_FILE)); +} + +int main() +{ + PRINT_TEST_BANNER(); + + assert_true(mkdtemp(CHROOT_DIR) != NULL); + SetChangesChroot(CHROOT_DIR); + + const UnitTest tests[] = + { + unit_test(test_diff_install_cancels_removal), + unit_test(test_manifest_install_cancels_removal), + }; + + int ret = run_tests(tests); + + DeleteDirectoryTree(CHROOT_DIR); + rmdir(CHROOT_DIR); + + return ret; +}