From ab0408f1f8f898e8f22b6a100d7106d46a4a3008 Mon Sep 17 00:00:00 2001 From: Daniel JB Clark Date: Tue, 10 Nov 2026 12:05:11 -0500 Subject: [PATCH] Fixed simulate diff mode aborting on repeated unversioned package installs DiffPkgOperations() compared the versions of two recorded installations of the same package without checking that both were set, which PkgVersionIsGreater() asserts. The diff report now keeps the record the way the manifest report does: a versioned record over an unversioned one, otherwise the higher version, otherwise the first one recorded. Ticket: none Changelog: Title Signed-off-by: Daniel JB Clark Co-Authored-By: Claude Fable 5.1 Co-Authored-By: Claude Fable 5.2 Co-Authored-By: Claude Opus 5.5 --- cf-agent/simulate_mode.c | 27 +++++++- tests/unit/simulate_mode_test.c | 114 ++++++++++++++++++++++++++++++-- 2 files changed, 135 insertions(+), 6 deletions(-) diff --git a/cf-agent/simulate_mode.c b/cf-agent/simulate_mode.c index 35264e62cc2..2b1d2995ab7 100644 --- a/cf-agent/simulate_mode.c +++ b/cf-agent/simulate_mode.c @@ -793,6 +793,31 @@ static inline char *GetPkgOperationMsg(ChrootPkgOperationCode op, const char *pk return msg; } +static bool PkgRecordShouldBeReplaced(const PkgOperationRecord *prev_record, + const char *new_ver) +{ + if (prev_record == NULL) + { + /* Nothing recorded yet */ + return true; + } + + if (NULL_OR_EMPTY(new_ver)) + { + /* An unversioned record never replaces an existing one */ + return false; + } + + if (NULL_OR_EMPTY(prev_record->pkg_ver)) + { + /* A versioned record is more specific than an unversioned one */ + return true; + } + + /* Both are versioned, keep the higher version */ + return PkgVersionIsGreater(new_ver, prev_record->pkg_ver); +} + bool DiffPkgOperations() { const char *pkgs_ops_csv_file = ToChangesChroot(CHROOT_PKGS_OPS_FILE); @@ -893,7 +918,7 @@ bool DiffPkgOperations() MapRemove(removed, name_arch); PkgOperationRecord *prev_record = MapGet(installed, name_arch); - if ((prev_record == NULL) || PkgVersionIsGreater(pkg_ver, prev_record->pkg_ver)) + if (PkgRecordShouldBeReplaced(prev_record, pkg_ver)) { char *msg = GetPkgOperationMsg(CHROOT_PKG_OPERATION_CODE_INSTALL, pkg_name, pkg_arch, pkg_ver); diff --git a/tests/unit/simulate_mode_test.c b/tests/unit/simulate_mode_test.c index 10b5513121a..9aa8cc01a1e 100644 --- a/tests/unit/simulate_mode_test.c +++ b/tests/unit/simulate_mode_test.c @@ -82,8 +82,8 @@ static void test_diff_install_cancels_removal(void) 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); + assert_true(StringContains(output, "Package 'foo [1.2.3]' would be installed")); + assert_false(StringContains(output, "would be removed")); unlink(ToChangesChroot(CHROOT_PKGS_OPS_FILE)); } @@ -98,7 +98,7 @@ static void test_diff_version_and_architecture(void) char output[4096]; assert_true(call_with_captured_stdout(&DiffPkgOperations, output, sizeof(output))); - assert_true(strstr(output, "Package 'foo-x86_64 [1.2.3]' would be installed") != NULL); + assert_true(StringContains(output, "Package 'foo-x86_64 [1.2.3]' would be installed")); unlink(ToChangesChroot(CHROOT_PKGS_OPS_FILE)); } @@ -141,8 +141,106 @@ static void test_manifest_install_cancels_removal(void) 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); + assert_true(StringContains(output, "Package 'foo [1.2.3]' would be present")); + assert_false(StringContains(output, "would be absent")); + + unlink(ToChangesChroot(CHROOT_PKGS_OPS_FILE)); +} + +/* Two recorded installations of the same package without a version must be + * reported once, not abort on comparing the empty versions. */ +static void test_diff_repeated_unversioned_install(void) +{ + write_pkgs_ops("i,foo,,\r\n" + "i,foo,,\r\n"); + + char output[4096]; + assert_true(call_with_captured_stdout(&DiffPkgOperations, output, sizeof(output))); + + assert_true(StringContains(output, "Package 'foo' would be installed")); + /* Reported once: the report has no other line. */ + assert_int_equal(StringCountTokens(output, strlen(output), "\n"), 1); + + unlink(ToChangesChroot(CHROOT_PKGS_OPS_FILE)); +} + +/* A versioned installation record is more specific than an unversioned one + * of the same package, so it is the one to report. */ +static void test_diff_unversioned_then_versioned_install(void) +{ + write_pkgs_ops("i,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(StringContains(output, "Package 'foo [1.2.3]' would be installed")); + assert_false(StringContains(output, "Package 'foo' would be installed")); + + unlink(ToChangesChroot(CHROOT_PKGS_OPS_FILE)); +} + +/* An unversioned installation record must never replace an existing + * versioned installation record of the same package. */ +static void test_diff_versioned_then_unversioned_install(void) +{ + write_pkgs_ops("i,foo,1.2.3,\r\n" + "i,foo,,\r\n"); + + char output[4096]; + assert_true(call_with_captured_stdout(&DiffPkgOperations, output, sizeof(output))); + + assert_true(StringContains(output, "Package 'foo [1.2.3]' would be installed")); + assert_false(StringContains(output, "Package 'foo' would be installed")); + + unlink(ToChangesChroot(CHROOT_PKGS_OPS_FILE)); +} + +/* When two installations are recorded with different versions and the first + * has the higher version, the first record is kept. */ +static void test_diff_install_higher_version_first_keeps_first(void) +{ + write_pkgs_ops("i,foo,2.0.0,\r\n" + "i,foo,1.0.0,\r\n"); + + char output[4096]; + assert_true(call_with_captured_stdout(&DiffPkgOperations, output, sizeof(output))); + + assert_true(StringContains(output, "Package 'foo [2.0.0]' would be installed")); + assert_false(StringContains(output, "Package 'foo [1.0.0]' would be installed")); + + unlink(ToChangesChroot(CHROOT_PKGS_OPS_FILE)); +} + +/* When two installations are recorded with different versions and the last + * has the higher version, the last record replaces the first. */ +static void test_diff_install_higher_version_last_keeps_last(void) +{ + write_pkgs_ops("i,foo,1.0.0,\r\n" + "i,foo,2.0.0,\r\n"); + + char output[4096]; + assert_true(call_with_captured_stdout(&DiffPkgOperations, output, sizeof(output))); + + assert_true(StringContains(output, "Package 'foo [2.0.0]' would be installed")); + assert_false(StringContains(output, "Package 'foo [1.0.0]' would be installed")); + + unlink(ToChangesChroot(CHROOT_PKGS_OPS_FILE)); +} + +/* Two recorded installations of the same package with the same version + * must be reported once. */ +static void test_diff_repeated_versioned_install(void) +{ + write_pkgs_ops("i,foo,1.2.3,\r\n" + "i,foo,1.2.3,\r\n"); + + char output[4096]; + assert_true(call_with_captured_stdout(&DiffPkgOperations, output, sizeof(output))); + + assert_true(StringContains(output, "Package 'foo [1.2.3]' would be installed")); + /* Reported once: the report has no other line. */ + assert_int_equal(StringCountTokens(output, strlen(output), "\n"), 1); unlink(ToChangesChroot(CHROOT_PKGS_OPS_FILE)); } @@ -161,6 +259,12 @@ int main() unit_test(test_diff_removal_with_version), unit_test(test_diff_removal_with_other_version_keeps_install), unit_test(test_manifest_install_cancels_removal), + unit_test(test_diff_repeated_unversioned_install), + unit_test(test_diff_unversioned_then_versioned_install), + unit_test(test_diff_versioned_then_unversioned_install), + unit_test(test_diff_install_higher_version_first_keeps_first), + unit_test(test_diff_install_higher_version_last_keeps_last), + unit_test(test_diff_repeated_versioned_install), }; int ret = run_tests(tests);