From 17396300fb85a1793a9b718215a7081cf0128b73 Mon Sep 17 00:00:00 2001 From: mssonicbld <79238446+mssonicbld@users.noreply.github.com> Date: Sat, 27 Jun 2026 19:13:21 +1000 Subject: [PATCH] Fix HFTelOrch double-delete of NotificationConsumer on shutdown (#4702) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit **What I did** Fixed a double-delete of the HFTel ASIC `NotificationConsumer` in `HFTelOrch` by aligning ownership with the orchagent `Executor` model. - Replaced `std::shared_ptr` with a non-owning `swss::NotificationConsumer*` in `hftelorch.h`. - Allocate the consumer with `new` and pass it directly to `Notifier` in `hftelorch.cpp` (same pattern as `PortsOrch`). - On failed `SAI_SWITCH_ATTR_TAM_TEL_TYPE_CONFIG_CHANGE_NOTIFY` registration, delete the `Notifier` and clear the pointer before throwing. - Updated `doTask(NotificationConsumer&)` to compare against the raw pointer instead of `shared_ptr::get()`. **Why I did it** `HFTelOrch` was introduced in `a18824e6` (#3759) with dual ownership: a `shared_ptr` member and a `Notifier`/`Executor` that also deletes `m_selectable` in `~Executor()`. On graceful shutdown, `~HFTelOrch` destroyed the consumer first and `~Orch` destroyed it again, causing SIGSEGV in `redisFree()` during `~NotificationConsumer`. This was observed on SONiC as repeated orchagent cores during stop/restart. The failure is most visible once full teardown runs (e.g. after graceful SIGTERM/SIGINT shutdown paths such as #4400), but the ownership bug has existed since HFTel was added. **How I verified it** - Code review against `orch.h` (`Executor` owns and deletes `m_selectable`) and `portsorch.cpp` (reference ownership pattern). - `git diff --check` on the changed files (no whitespace issues). - Prior GDB analysis on DUT: stack showed double `~NotificationConsumer`, `redisFree()`, and `std::_Sp_counted_base::_M_release()`; no SAI failure in `sairedis.rec` at crash time. Recommended DUT verification (post-build): 1. Confirm HFTel is enabled: `sudo grep -i "High Frequency Telemetry" /var/log/syslog` 2. Stop/restart orchagent repeatedly: `docker exec swss supervisorctl stop orchagent` (or restart `swss`) 3. Confirm no new cores: `ls -lt /var/core/orchagent*.core*` **Details if related** - Introduced by: `a18824e6` — [orchagent]: HFTOrch init (#3759) - Often exposed by: graceful orchagent shutdown / repeated restarts (related: `bd39b131` — [orchagent] Async swss.rec (#4400)) - Scope: `orchagent/high_frequency_telemetry/hftelorch.{h,cpp}` only - Do not delete the consumer in `~HFTelOrch`; `~Executor` handles it after `addExecutor()` - Observed: six identical orchagent cores on DUT over ~48 minutes (2026-05-19); Fixed the issue https://github.com/sonic-net/sonic-buildimage/issues/27464 Signed-off-by: Sonic Build Admin --- .../high_frequency_telemetry/hftelorch.cpp | 8 +- .../high_frequency_telemetry/hftelorch.h | 2 +- tests/mock_tests/hftelorch_ut.cpp | 142 ++++++++++++++++++ 3 files changed, 148 insertions(+), 4 deletions(-) diff --git a/orchagent/high_frequency_telemetry/hftelorch.cpp b/orchagent/high_frequency_telemetry/hftelorch.cpp index 465c1481..77aea982 100644 --- a/orchagent/high_frequency_telemetry/hftelorch.cpp +++ b/orchagent/high_frequency_telemetry/hftelorch.cpp @@ -78,14 +78,16 @@ HFTelOrch::HFTelOrch( createNetlinkChannel("sonic_stel", "ipfix"); createTAM(); - m_asic_notification_consumer = make_shared(&m_asic_db, "NOTIFICATIONS"); - auto notifier = new Notifier(m_asic_notification_consumer.get(), this, "TAM_TEL_TYPE_STATE"); + m_asic_notification_consumer = new NotificationConsumer(&m_asic_db, "NOTIFICATIONS"); + auto notifier = new Notifier(m_asic_notification_consumer, this, "TAM_TEL_TYPE_STATE"); sai_attribute_t attr; attr.id = SAI_SWITCH_ATTR_TAM_TEL_TYPE_CONFIG_CHANGE_NOTIFY; attr.value.ptr = (void *)on_tam_tel_type_config_change; if (sai_switch_api->set_switch_attribute(gSwitchId, &attr) != SAI_STATUS_SUCCESS) { SWSS_LOG_ERROR("Failed to set SAI_SWITCH_ATTR_TAM_TEL_TYPE_CONFIG_CHANGE_NOTIFY"); + delete notifier; + m_asic_notification_consumer = nullptr; throw runtime_error("HFTelOrch initialization failure (failed to set tam tel type config change notify)"); } @@ -490,7 +492,7 @@ void HFTelOrch::doTask(swss::NotificationConsumer &consumer) std::string data; std::vector values; - if (&consumer != m_asic_notification_consumer.get()) + if (&consumer != m_asic_notification_consumer) { SWSS_LOG_DEBUG("Is not TAM notification"); return; diff --git a/orchagent/high_frequency_telemetry/hftelorch.h b/orchagent/high_frequency_telemetry/hftelorch.h index 99e67d9a..845a18fa 100644 --- a/orchagent/high_frequency_telemetry/hftelorch.h +++ b/orchagent/high_frequency_telemetry/hftelorch.h @@ -34,7 +34,7 @@ class HFTelOrch : public Orch private: swss::Table m_state_telemetry_session; swss::DBConnector m_asic_db; - std::shared_ptr m_asic_notification_consumer; + swss::NotificationConsumer* m_asic_notification_consumer = nullptr; std::unordered_map> m_name_profile_mapping; std::unordered_map>> m_type_profile_mapping; diff --git a/tests/mock_tests/hftelorch_ut.cpp b/tests/mock_tests/hftelorch_ut.cpp index c0a3a656..0050cfc4 100644 --- a/tests/mock_tests/hftelorch_ut.cpp +++ b/tests/mock_tests/hftelorch_ut.cpp @@ -2,13 +2,49 @@ #include "ut_helper.h" #include "mock_orchagent_main.h" #include "high_frequency_telemetry/hftelorch.h" +#include "schema.h" #include +#include + +extern sai_switch_api_t *sai_switch_api; namespace hftelorch_test { using namespace std; using hftel_is_supported_ut::SaiHookGuard; + namespace constructor_ut + { + sai_switch_api_t *pold_sai_switch_api = nullptr; + sai_switch_api_t ut_sai_switch_api{}; + + sai_status_t _ut_stub_sai_set_switch_attribute( + _In_ sai_object_id_t switch_id, + _In_ const sai_attribute_t *attr) + { + if (attr->id == SAI_SWITCH_ATTR_TAM_TEL_TYPE_CONFIG_CHANGE_NOTIFY) + { + return SAI_STATUS_FAILURE; + } + + return pold_sai_switch_api->set_switch_attribute(switch_id, attr); + } + + void hookSaiSwitchApi() + { + ut_sai_switch_api = *sai_switch_api; + pold_sai_switch_api = sai_switch_api; + ut_sai_switch_api.set_switch_attribute = _ut_stub_sai_set_switch_attribute; + sai_switch_api = &ut_sai_switch_api; + } + + void unhookSaiSwitchApi() + { + sai_switch_api = pold_sai_switch_api; + pold_sai_switch_api = nullptr; + } + } + class HFTelOrchIsSupportedTest : public ::testing::Test { protected: @@ -91,4 +127,110 @@ namespace hftelorch_test SaiHookGuard guard(hftel_is_supported_ut::setSaiHookSwitchNotifySetNotImplemented); EXPECT_FALSE(HFTelOrch::isSupportedHFTel(gSwitchId)); } + + class HFTelOrchConstructorTest : public ::testing::Test + { + protected: + shared_ptr m_config_db; + shared_ptr m_state_db; + + void SetUp() override + { + map profile = { + {"SAI_VS_SWITCH_TYPE", "SAI_VS_SWITCH_TYPE_BCM56850"}, + {"KV_DEVICE_MAC_ADDRESS", "20:03:04:05:06:00"}, + }; + + ASSERT_EQ(ut_helper::initSaiApi(profile), SAI_STATUS_SUCCESS); + + sai_attribute_t attr{}; + attr.id = SAI_SWITCH_ATTR_INIT_SWITCH; + attr.value.booldata = true; + + ASSERT_EQ(sai_switch_api->create_switch(&gSwitchId, 1, &attr), SAI_STATUS_SUCCESS); + + m_config_db = make_shared("CONFIG_DB", 0); + m_state_db = make_shared("STATE_DB", 0); + + constructor_ut::hookSaiSwitchApi(); + } + + void TearDown() override + { + constructor_ut::unhookSaiSwitchApi(); + + ASSERT_EQ(sai_switch_api->remove_switch(gSwitchId), SAI_STATUS_SUCCESS); + gSwitchId = SAI_NULL_OBJECT_ID; + + ASSERT_EQ(ut_helper::uninitSaiApi(), SAI_STATUS_SUCCESS); + } + }; + + /* + * Forces set_switch_attribute for TAM_TEL_TYPE_CONFIG_CHANGE_NOTIFY to fail. + * Covers constructor error cleanup: delete notifier and nullptr consumer. + */ + TEST_F(HFTelOrchConstructorTest, ConstructorFailsWhenTamNotifySetFails) + { + const vector stel_tables = { + CFG_HIGH_FREQUENCY_TELEMETRY_PROFILE_TABLE_NAME, + CFG_HIGH_FREQUENCY_TELEMETRY_GROUP_TABLE_NAME, + }; + + EXPECT_THROW( + { + HFTelOrch orch(m_config_db.get(), m_state_db.get(), stel_tables); + (void)orch; + }, + runtime_error); + } + + class HFTelOrchShutdownTest : public ::testing::Test + { + protected: + shared_ptr m_config_db; + shared_ptr m_state_db; + + void SetUp() override + { + map profile = { + {"SAI_VS_SWITCH_TYPE", "SAI_VS_SWITCH_TYPE_BCM56850"}, + {"KV_DEVICE_MAC_ADDRESS", "20:03:04:05:06:00"}, + }; + + ASSERT_EQ(ut_helper::initSaiApi(profile), SAI_STATUS_SUCCESS); + + sai_attribute_t attr{}; + attr.id = SAI_SWITCH_ATTR_INIT_SWITCH; + attr.value.booldata = true; + + ASSERT_EQ(sai_switch_api->create_switch(&gSwitchId, 1, &attr), SAI_STATUS_SUCCESS); + + m_config_db = make_shared("CONFIG_DB", 0); + m_state_db = make_shared("STATE_DB", 0); + } + + void TearDown() override + { + ASSERT_EQ(sai_switch_api->remove_switch(gSwitchId), SAI_STATUS_SUCCESS); + gSwitchId = SAI_NULL_OBJECT_ID; + + ASSERT_EQ(ut_helper::uninitSaiApi(), SAI_STATUS_SUCCESS); + } + }; + + /* + * Successful ctor then dtor: Notifier/Executor owns the ASIC NotificationConsumer. + * Regression for double-delete on shutdown (shared_ptr member + ~Executor). + */ + TEST_F(HFTelOrchShutdownTest, DestructorDoesNotDoubleDeleteNotificationConsumer) + { + const vector stel_tables = { + CFG_HIGH_FREQUENCY_TELEMETRY_PROFILE_TABLE_NAME, + CFG_HIGH_FREQUENCY_TELEMETRY_GROUP_TABLE_NAME, + }; + + auto orch = make_unique(m_config_db.get(), m_state_db.get(), stel_tables); + orch.reset(); + } }