From 48d10a6decd29f39390094116c7680f91cb740a7 Mon Sep 17 00:00:00 2001 From: purush-nexthop Date: Tue, 26 May 2026 11:18:04 -0700 Subject: [PATCH 1/4] Fix null-deref crash when GLOBAL key is deleted Signed-off-by: purush-nexthop --- orchagent/pfcwdorch.cpp | 6 ++- tests/test_pfcwd.py | 85 +++++++++++++++++++++++++++++++++++++++++ 2 files changed, 90 insertions(+), 1 deletion(-) diff --git a/orchagent/pfcwdorch.cpp b/orchagent/pfcwdorch.cpp index a8f2e9f8f55..b66c4ff0418 100644 --- a/orchagent/pfcwdorch.cpp +++ b/orchagent/pfcwdorch.cpp @@ -325,7 +325,11 @@ task_process_status PfcWdOrch::deleteEntry(const st SWSS_LOG_ENTER(); Port port; - gPortsOrch->getPort(name, port); + if (!gPortsOrch->getPort(name, port)) + { + SWSS_LOG_ERROR("Invalid port interface %s", name.c_str()); + return task_process_status::task_invalid_entry; + } if (!stopWdOnPort(port)) { diff --git a/tests/test_pfcwd.py b/tests/test_pfcwd.py index c88b6f6e96b..c7ca29a4618 100644 --- a/tests/test_pfcwd.py +++ b/tests/test_pfcwd.py @@ -295,6 +295,91 @@ def test_pfcwd_software_multi_queue(self, dvs, setup_teardown_test): self.reset_pfcwd_counters(test_queues) self.stop_pfcwd_on_ports() + def test_pfcwd_global_key_del_no_crash(self, dvs, setup_teardown_test): + """ + Verify orchagent does not crash when PFC_WD|GLOBAL is deleted from CONFIG_DB. + + Deleting PFC_WD|GLOBAL fires a "del" keyspace notification which routes to + deleteEntry("GLOBAL"). Before the fix, getPort("GLOBAL") failed silently, + leaving a default-constructed Port with empty m_queue_ids. unregisterFromWdDb() + then accessed m_queue_ids[0] on the empty vector (null data pointer), causing + SIGSEGV at address 0. After the fix, deleteEntry() checks the getPort() return + value and returns task_invalid_entry cleanly for non-port keys. + """ + storm_queue = [3] + try: + test_queues = [3, 4] + self.set_ports_pfc(pfc_queues=test_queues) + self.verify_ports_pfc(test_queues) + + # Start pfcwd — writes POLL_INTERVAL to PFC_WD|GLOBAL + self.start_pfcwd_on_ports() + time.sleep(1) + + # Delete PFC_WD|GLOBAL entirely. + # Fires "del" keyspace notification → deleteEntry("GLOBAL") in orchagent. + # Before fix: SIGSEGV in unregisterFromWdDb() accessing null m_queue_ids[0]. + self.config_db.delete_entry("PFC_WD", "GLOBAL") + time.sleep(1) + + # Verify orchagent survived: restore GLOBAL and confirm pfcwd still works + self.config_db.update_entry("PFC_WD", "GLOBAL", {"POLL_INTERVAL": "200"}) + time.sleep(1) + + self.set_storm_state(storm_queue) + self.verify_pfcwd_state(storm_queue) + self.verify_pfcwd_counters(storm_queue) + + finally: + self.set_storm_state(storm_queue, state="disabled") + self.reset_pfcwd_counters(storm_queue) + self.stop_pfcwd_on_ports() + self.config_db.delete_entry("PFC_WD", "GLOBAL") + + def test_pfcwd_global_hdel_last_field_no_crash(self, dvs, setup_teardown_test): + """ + Verify orchagent does not crash when hdel removes the last field of PFC_WD|GLOBAL. + + With Redis notify-keyspace-events=AKE (which includes 'g' for generic commands), + hdel of the last field causes Redis to auto-delete the key and fire both "hdel" + and "del" keyspace notifications. The "del" notification triggers + deleteEntry("GLOBAL"), hitting the same crash path as test_pfcwd_global_key_del_no_crash. + """ + storm_queue = [3] + try: + test_queues = [3, 4] + self.set_ports_pfc(pfc_queues=test_queues) + self.verify_ports_pfc(test_queues) + + self.start_pfcwd_on_ports() + time.sleep(1) + + # Replace GLOBAL entry with only BIG_RED_SWITCH so hdel of it is the last field + self.config_db.delete_entry("PFC_WD", "GLOBAL") + time.sleep(0.5) + self.config_db.update_entry("PFC_WD", "GLOBAL", {"BIG_RED_SWITCH": "enable"}) + time.sleep(1) + + # Hdel the only remaining field. + # With AKE keyspace events, Redis fires "hdel" + "del" → deleteEntry("GLOBAL"). + self.config_db.delete_field("PFC_WD", "GLOBAL", "BIG_RED_SWITCH") + time.sleep(1) + + # Verify orchagent survived: restore GLOBAL and confirm pfcwd still works + self.config_db.update_entry("PFC_WD", "GLOBAL", {"POLL_INTERVAL": "200"}) + time.sleep(1) + + self.set_storm_state(storm_queue) + self.verify_pfcwd_state(storm_queue) + self.verify_pfcwd_counters(storm_queue) + + finally: + self.set_storm_state(storm_queue, state="disabled") + self.reset_pfcwd_counters(storm_queue) + self.stop_pfcwd_on_ports() + self.config_db.delete_entry("PFC_WD", "GLOBAL") + + # # Add Dummy always-pass test at end as workaroud # for issue when Flaky fail on final test it invokes module tear-down before retrying From 49efa300564f4945640a8c754d27d090634e8d6d Mon Sep 17 00:00:00 2001 From: Pinky Agrawal Date: Wed, 22 Jul 2026 12:17:51 +0530 Subject: [PATCH 2/4] fix the test failures test_pfcwd_global_hdel_last_field_no_crash exercised PFC_WD|GLOBAL delete/hdel paths but its post-conditions were contaminated by the BIG_RED_SWITCH field it used to build a single-field GLOBAL entry: - BIG_RED_SWITCH:enable puts pfcwdorch into big-red-switch mode, which force-drops monitored queues and bypasses normal storm detection. Deleting the field does not transition BRS off, so the queue stayed in force-drop mode and verify_pfcwd_state saw PFC_WD_STATUS=operational instead of stormed. Explicitly disable BIG_RED_SWITCH and reset GLOBAL to a clean POLL_INTERVAL before verifying storm detection. - The BRS enable/disable cycle also bumps the cumulative DEADLOCK_DETECTED/RESTORED counters, so verify_pfcwd_counters saw 2/2 instead of the expected 1/0. Reset the counters before the storm so the single storm yields detected=1, restored=0. Signed-off-by: Pinky Agrawal --- tests/test_pfcwd.py | 16 +++++++++++++++- 1 file changed, 15 insertions(+), 1 deletion(-) diff --git a/tests/test_pfcwd.py b/tests/test_pfcwd.py index c7ca29a4618..1fa48ff6f7e 100644 --- a/tests/test_pfcwd.py +++ b/tests/test_pfcwd.py @@ -365,10 +365,24 @@ def test_pfcwd_global_hdel_last_field_no_crash(self, dvs, setup_teardown_test): self.config_db.delete_field("PFC_WD", "GLOBAL", "BIG_RED_SWITCH") time.sleep(1) - # Verify orchagent survived: restore GLOBAL and confirm pfcwd still works + # Verify orchagent survived: restore GLOBAL and confirm pfcwd still works. + # BIG_RED_SWITCH:enable above put pfcwdorch into big-red-switch mode, which + # force-drops monitored queues and bypasses normal storm detection. Deleting + # the field does not transition BRS off, so explicitly disable it before + # verifying storm detection, otherwise the queue stays operational. + self.config_db.update_entry("PFC_WD", "GLOBAL", {"BIG_RED_SWITCH": "disable"}) + time.sleep(1) + self.config_db.delete_entry("PFC_WD", "GLOBAL") + time.sleep(1) self.config_db.update_entry("PFC_WD", "GLOBAL", {"POLL_INTERVAL": "200"}) time.sleep(1) + # The BIG_RED_SWITCH enable/disable cycle above force-drops and restores the + # monitored queues, bumping the cumulative DEADLOCK_DETECTED/RESTORED counters. + # Reset them so the single storm below yields the expected detected=1, + # restored=0 that verify_pfcwd_counters checks. + self.reset_pfcwd_counters(storm_queue) + self.set_storm_state(storm_queue) self.verify_pfcwd_state(storm_queue) self.verify_pfcwd_counters(storm_queue) From 5affe76ee2ec1227518e4f2cb9f66419ecb96733 Mon Sep 17 00:00:00 2001 From: Pinky Agrawal Date: Thu, 23 Jul 2026 09:47:38 +0530 Subject: [PATCH 3/4] [pfcwdorch] Special-case GLOBAL key in deleteEntry Address review: GLOBAL is a valid config key, not a port, so deleting PFC_WD|GLOBAL should not be logged as an "Invalid port interface" error. Short-circuit the GLOBAL key to task_success before the port lookup, mirroring the existing createEntry override. Genuinely invalid port keys still return task_invalid_entry with the error. Update the test docstring to match. Signed-off-by: Pinky Agrawal --- orchagent/pfcwdorch.cpp | 10 ++++++++++ tests/test_pfcwd.py | 5 +++-- 2 files changed, 13 insertions(+), 2 deletions(-) diff --git a/orchagent/pfcwdorch.cpp b/orchagent/pfcwdorch.cpp index b66c4ff0418..8aefd244f3d 100644 --- a/orchagent/pfcwdorch.cpp +++ b/orchagent/pfcwdorch.cpp @@ -324,6 +324,16 @@ task_process_status PfcWdOrch::deleteEntry(const st { SWSS_LOG_ENTER(); + // GLOBAL is a valid config key (handled specially in createEntry), not a port. + // Deleting PFC_WD|GLOBAL is a legitimate no-op for the per-port teardown path, + // so short-circuit it here before the port lookup. This both avoids the + // null-deref crash and prevents a spurious "Invalid port interface" error on + // routine GLOBAL removal. + if (name == PFC_WD_GLOBAL) + { + return task_process_status::task_success; + } + Port port; if (!gPortsOrch->getPort(name, port)) { diff --git a/tests/test_pfcwd.py b/tests/test_pfcwd.py index 1fa48ff6f7e..a0b23e2df30 100644 --- a/tests/test_pfcwd.py +++ b/tests/test_pfcwd.py @@ -303,8 +303,9 @@ def test_pfcwd_global_key_del_no_crash(self, dvs, setup_teardown_test): deleteEntry("GLOBAL"). Before the fix, getPort("GLOBAL") failed silently, leaving a default-constructed Port with empty m_queue_ids. unregisterFromWdDb() then accessed m_queue_ids[0] on the empty vector (null data pointer), causing - SIGSEGV at address 0. After the fix, deleteEntry() checks the getPort() return - value and returns task_invalid_entry cleanly for non-port keys. + SIGSEGV at address 0. After the fix, deleteEntry() short-circuits the GLOBAL + key (returning task_success) before the port lookup, so the null-deref path is + never reached; genuinely invalid port keys still return task_invalid_entry. """ storm_queue = [3] try: From aba9491e8e03067b2bbaac4bfd87a96e55e42ec5 Mon Sep 17 00:00:00 2001 From: Pinky Agrawal Date: Thu, 23 Jul 2026 16:43:30 +0530 Subject: [PATCH 4/4] [tests] Cover deleteEntry invalid-port branch Add test_pfcwd_del_unknown_port_no_crash: deleting a PFC_WD entry for a non-existent port drives the getPort() failure path (task_invalid_entry), which the GLOBAL short-circuit no longer reaches. Restores diff coverage on deleteEntry to 100%. Signed-off-by: Pinky Agrawal --- tests/test_pfcwd.py | 35 +++++++++++++++++++++++++++++++++++ 1 file changed, 35 insertions(+) diff --git a/tests/test_pfcwd.py b/tests/test_pfcwd.py index a0b23e2df30..2e71a23c2d8 100644 --- a/tests/test_pfcwd.py +++ b/tests/test_pfcwd.py @@ -394,6 +394,41 @@ def test_pfcwd_global_hdel_last_field_no_crash(self, dvs, setup_teardown_test): self.stop_pfcwd_on_ports() self.config_db.delete_entry("PFC_WD", "GLOBAL") + def test_pfcwd_del_unknown_port_no_crash(self, dvs, setup_teardown_test): + """ + Verify orchagent handles deletion of a PFC_WD entry for an unknown/invalid + port gracefully, exercising the invalid-key guard in deleteEntry(). + """ + storm_queue = [3] + invalid_port = "Ethernet9999" + try: + self.start_pfcwd_on_ports() + time.sleep(1) + + self.config_db.create_entry("PFC_WD", invalid_port, + {"action": "drop", + "detection_time": "200", + "restoration_time": "200"}) + time.sleep(1) + self.config_db.delete_entry("PFC_WD", invalid_port) + time.sleep(1) + + # Verify orchagent survived and pfcwd still works on real ports. + test_queues = [3, 4] + self.set_ports_pfc(pfc_queues=test_queues) + self.verify_ports_pfc(test_queues) + self.reset_pfcwd_counters(storm_queue) + self.set_storm_state(storm_queue) + self.verify_pfcwd_state(storm_queue) + self.verify_pfcwd_counters(storm_queue) + + finally: + self.set_storm_state(storm_queue, state="disabled") + self.reset_pfcwd_counters(storm_queue) + self.stop_pfcwd_on_ports() + self.config_db.delete_entry("PFC_WD", invalid_port) + self.config_db.delete_entry("PFC_WD", "GLOBAL") + # # Add Dummy always-pass test at end as workaroud