From 2044c5c6576938011e33c03a5cbf40b101f6ebba Mon Sep 17 00:00:00 2001 From: Martin Vogel Date: Tue, 4 Aug 2026 21:35:24 +0200 Subject: [PATCH] fix(cli): attribute activation refusals to the recorded safety check (#1416) Windows users hit 'active CBM sessions and operations could not be stopped safely; no activation was committed' from install, uninstall and doctor with ZERO CBM processes running - they rebooted, killed phantom handles, and deleted runtime folders chasing sessions that did not exist. The real failure was the activation transaction's Windows ACL safety check refusing a directory carrying cross-account mutation grants (commonly the stock Authenticated Users:(M) inheritance from a drive root - reproduced on the Windows VM with a plain mkdir under C:\ and with an icacls-granted parent), and the refusal detail was recorded internally but never shown. cli_activation_diagnostic now prefixes the recorded refusal note - the predicate, SID and path - plus one remediation line (remove the flagged grant or use an owner-private directory). The sessions wording remains for genuine stop/reservation failures, which record no note. Adds a CBM_ENABLE_TEST_SEAMS setter for the refusal note so the attribution is testable portably; the regression test asserts the note reaches the diagnostic (and that the no-note path keeps the sessions text). RED before the fix, GREEN after, RED again on revert. Verified end-to-end on the Windows VM: uninstall/doctor against an ACL-tainted tree now name the ACL check instead of sessions. Signed-off-by: Martin Vogel --- src/cli/activation_transaction.c | 12 +++++++++++ src/cli/activation_transaction.h | 5 +++++ src/cli/cli.c | 18 ++++++++++++++++ tests/test_cli.c | 37 ++++++++++++++++++++++++++++++++ 4 files changed, 72 insertions(+) diff --git a/src/cli/activation_transaction.c b/src/cli/activation_transaction.c index 90cda8159..9ea1cfc6b 100644 --- a/src/cli/activation_transaction.c +++ b/src/cli/activation_transaction.c @@ -110,6 +110,18 @@ const char *cbm_activation_transaction_refusal_note(void) { return g_activation_refusal_note; } +#ifdef CBM_ENABLE_TEST_SEAMS +/* #1416 test seam: install a refusal note so the CLI attribution path is + * testable without constructing a real Windows ACL refusal. */ +void cbm_activation_transaction_note_refusal_for_testing(const char *predicate, + unsigned long os_error) { + activation_refusal_clear(); + if (predicate && predicate[0]) { + activation_note_refusal(predicate, os_error); + } +} +#endif + #ifdef _WIN32 typedef HANDLE activation_native_file_t; #define ACTIVATION_INVALID_FILE INVALID_HANDLE_VALUE diff --git a/src/cli/activation_transaction.h b/src/cli/activation_transaction.h index e3847ea46..c0dd89bfc 100644 --- a/src/cli/activation_transaction.h +++ b/src/cli/activation_transaction.h @@ -97,4 +97,9 @@ const char *cbm_activation_transaction_status_message(cbm_activation_transaction * entry; single-threaded like the rest of the transaction API. */ const char *cbm_activation_transaction_refusal_note(void); +#ifdef CBM_ENABLE_TEST_SEAMS +void cbm_activation_transaction_note_refusal_for_testing(const char *predicate, + unsigned long os_error); +#endif + #endif /* CBM_ACTIVATION_TRANSACTION_H */ diff --git a/src/cli/cli.c b/src/cli/cli.c index 70f8859a5..e5a6346c2 100644 --- a/src/cli/cli.c +++ b/src/cli/cli.c @@ -197,6 +197,24 @@ static const char *g_cli_activation_runtime_parent_for_test = NULL; static void cli_activation_diagnostic(const cbm_cli_activation_ops_t *ops, const char *message) { const char *diagnostic = message ? message : CLI_ACTIVATION_REFUSED_MESSAGE; + /* #1416: when the transaction recorded a concrete refusal (an ACL or + * filesystem safety check), say THAT. The generic text blames "active CBM + * sessions" for what is a validation refusal - reporters rebooted, killed + * every process, and hunted phantom handles because the message pointed at + * sessions that did not exist. The sessions wording remains for genuine + * stop/reservation failures, which record no refusal note. */ + char attributed[CBM_SZ_1K]; + const char *note = cbm_activation_transaction_refusal_note(); + if (diagnostic == CLI_ACTIVATION_REFUSED_MESSAGE && note && note[0]) { + (void)snprintf(attributed, sizeof(attributed), + "error: activation was refused by a filesystem safety check before any " + "change was made: %s\n" + "error: this is not a session problem. If the flagged directory is one you " + "trust, remove the flagged permission grant (icacls /remove:g ) " + "or use an owner-private directory for --dir/CBM_CACHE_DIR, then retry.", + note); + diagnostic = attributed; + } if (ops && ops->visible_diagnostic) { ops->visible_diagnostic(ops->context, diagnostic); return; diff --git a/tests/test_cli.c b/tests/test_cli.c index c1f9d4e82..467c221af 100644 --- a/tests/test_cli.c +++ b/tests/test_cli.c @@ -14,6 +14,7 @@ #include "test_framework.h" #include "test_helpers.h" #include +#include #include #include #include @@ -646,6 +647,41 @@ TEST(cli_activation_refuses_when_cohort_does_not_drain) { PASS(); } +/* Regression for #1416: when the activation transaction recorded a concrete + * refusal (e.g. the Windows ACL safety check), the CLI must attribute the + * failure to that check instead of blaming "active CBM sessions" - reporters + * rebooted and hunted phantom handles because no sessions existed. The + * sessions wording must remain for refusals with no recorded note. */ +TEST(cli_activation_refusal_note_reaches_diagnostic_issue1416) { + cbm_activation_transaction_note_refusal_for_testing( + "acl-grants-cross-account-mutation to S-1-5-11", 0UL); + cli_activation_fake_t fake = { + .participants_active = true, + .mutation_reserve_result = 0, + }; + cbm_cli_activation_ops_t ops = { + .context = &fake, + .reserve_for_mutation = cli_activation_fake_reserve_mutation, + .mutation_lease_release = cli_activation_fake_release_mutation, + .visible_diagnostic = cli_activation_fake_diagnostic, + }; + ASSERT_EQ(cbm_cli_activation_guard_with_ops(&ops, cli_activation_fake_mutation, &fake), 1); + ASSERT_NOT_NULL(strstr(fake.diagnostic, "acl-grants-cross-account-mutation")); + ASSERT_NOT_NULL(strstr(fake.diagnostic, "not a session problem")); + ASSERT_NULL(strstr(fake.diagnostic, "could not be stopped safely")); + + /* No note recorded -> the sessions wording is still the right message. */ + cbm_activation_transaction_note_refusal_for_testing(NULL, 0UL); + cli_activation_fake_t plain = { + .participants_active = true, + .mutation_reserve_result = 0, + }; + ops.context = &plain; + ASSERT_EQ(cbm_cli_activation_guard_with_ops(&ops, cli_activation_fake_mutation, &plain), 1); + ASSERT_NOT_NULL(strstr(plain.diagnostic, "could not be stopped safely")); + PASS(); +} + TEST(cli_activation_refuses_unsafe_cohort_reservation) { cli_activation_fake_t fake = { .mutation_reserve_result = -1, @@ -11789,6 +11825,7 @@ SUITE(cli) { /* Mandatory daemon activation safety */ RUN_TEST(cli_activation_quiesces_active_cohort_before_mutation); RUN_TEST(cli_activation_refuses_when_cohort_does_not_drain); + RUN_TEST(cli_activation_refusal_note_reaches_diagnostic_issue1416); RUN_TEST(cli_activation_refuses_unsafe_cohort_reservation); RUN_TEST(cli_activation_releases_maintenance_lease_after_success); RUN_TEST(cli_activation_releases_maintenance_lease_when_mutation_fails);