From a7ce6055b75d32f4d28aac02d0fce541f62a30b3 Mon Sep 17 00:00:00 2001 From: Jan Tore Guggedal Date: Fri, 27 Jun 2025 14:57:04 +0200 Subject: [PATCH] zbus: Fix NULL pointer use in zbus_chan_rm_obs() Fix a bug in zbus_chan_rm_obs() where removing the first observer in a channel's observer list would cause undefined behavior due to accessing a member of a NULL pointer. The issue occurred when prev_obs_nd was NULL (indicating the first node in the list) and the code attempted to pass &prev_obs_nd->node to sys_slist_remove(). This resulted in accessing the 'node' member of a NULL pointer, which is undefined behavior even when taking its address. The sys_slist_remove() function is designed to handle a NULL prev_node parameter correctly for removing the first element in a list. The fix ensures we pass NULL directly instead of attempting to compute the address of a member within a NULL pointer. This was detected by Undefined Behavior Sanitizer as "member access within null pointer". Signed-off-by: Jan Tore Guggedal --- subsys/zbus/zbus_runtime_observers.c | 4 +- .../runtime_observers_registration/src/main.c | 65 ++++++++++++++++++ .../src/main.c | 66 +++++++++++++++++++ 3 files changed, 134 insertions(+), 1 deletion(-) diff --git a/subsys/zbus/zbus_runtime_observers.c b/subsys/zbus/zbus_runtime_observers.c index fac25ac4b72..122d094ab68 100644 --- a/subsys/zbus/zbus_runtime_observers.c +++ b/subsys/zbus/zbus_runtime_observers.c @@ -175,7 +175,9 @@ int zbus_chan_rm_obs(const struct zbus_channel *chan, const struct zbus_observer SYS_SLIST_FOR_EACH_CONTAINER_SAFE(&chan->data->observers, obs_nd, tmp, node) { if (obs_nd->obs == obs) { - sys_slist_remove(&chan->data->observers, &prev_obs_nd->node, &obs_nd->node); + sys_slist_remove(&chan->data->observers, + prev_obs_nd ? &prev_obs_nd->node : NULL, + &obs_nd->node); #if defined(CONFIG_ZBUS_RUNTIME_OBSERVERS_NODE_ALLOC_NONE) obs_nd->chan = NULL; #else diff --git a/tests/subsys/zbus/runtime_observers_registration/src/main.c b/tests/subsys/zbus/runtime_observers_registration/src/main.c index 926e5a6ed8b..c1b75f07bc0 100644 --- a/tests/subsys/zbus/runtime_observers_registration/src/main.c +++ b/tests/subsys/zbus/runtime_observers_registration/src/main.c @@ -210,4 +210,69 @@ ZTEST(basic, test_specification_based__zbus_obs_priority) zassert_equal(execution_sequence[5], 1); } +/** + * @brief Test removing the first observer in a runtime observer list. + */ +ZTEST(basic, test_remove_first_runtime_observer) +{ + struct sensor_data_msg sd = {.a = 42, .b = 84}; + + /* Reset callback counters */ + count_callback1 = 0; + count_callback2 = 0; + + /* Start with an empty channel (chan3) */ + zassert_equal(0, zbus_chan_pub(&chan3, &sd, K_MSEC(500))); + zassert_equal(count_callback1, 0, "No observers should be called initially"); + zassert_equal(count_callback2, 0, "No observers should be called initially"); + + /* Add multiple observers to create a list */ + zassert_equal(0, zbus_chan_add_obs(&chan3, &lis1, K_MSEC(200)), + "First observer should be added successfully"); + zassert_equal(0, zbus_chan_add_obs(&chan3, &lis3, K_MSEC(200)), + "Second observer should be added successfully"); + zassert_equal(0, zbus_chan_add_obs(&chan3, &lis4, K_MSEC(200)), + "Third observer should be added successfully"); + + /* Verify all observers are called */ + zassert_equal(0, zbus_chan_pub(&chan3, &sd, K_MSEC(500))); + zassert_equal(count_callback1, 1, "lis1 callback should be called once"); + zassert_equal(count_callback2, 2, "lis3 and lis4 callbacks should be called"); + + /* Reset counters for the main test */ + count_callback1 = 0; + count_callback2 = 0; + + /* Remove the first observer in the list (lis1) */ + zassert_equal(0, zbus_chan_rm_obs(&chan3, &lis1, K_MSEC(200)), + "First observer should be removed successfully"); + + /* Verify only the remaining observers are called */ + zassert_equal(0, zbus_chan_pub(&chan3, &sd, K_MSEC(500))); + zassert_equal(count_callback1, 0, "lis1 callback should not be called after removal"); + zassert_equal(count_callback2, 2, "lis3 and lis4 callbacks should still be called"); + + /* Remove the new first observer (lis3) to test the same scenario again */ + count_callback2 = 0; + zassert_equal(0, zbus_chan_rm_obs(&chan3, &lis3, K_MSEC(200)), + "New first observer should be removed successfully"); + + /* Verify only lis4 is called */ + zassert_equal(0, zbus_chan_pub(&chan3, &sd, K_MSEC(500))); + zassert_equal(count_callback2, 1, "Only lis4 callback should be called"); + + /* Remove the last observer */ + count_callback2 = 0; + zassert_equal(0, zbus_chan_rm_obs(&chan3, &lis4, K_MSEC(200)), + "Last observer should be removed successfully"); + + /* Verify no observers are called */ + zassert_equal(0, zbus_chan_pub(&chan3, &sd, K_MSEC(500))); + zassert_equal(count_callback2, 0, "No callbacks should be called after all removed"); + + /* Test error case: try to remove non-existent observer */ + zassert_equal(-ENODATA, zbus_chan_rm_obs(&chan3, &lis1, K_MSEC(200)), + "Removing non-existent observer should return -ENODATA"); +} + ZTEST_SUITE(basic, NULL, NULL, NULL, NULL, NULL); diff --git a/tests/subsys/zbus/runtime_observers_registration_alloc_none/src/main.c b/tests/subsys/zbus/runtime_observers_registration_alloc_none/src/main.c index 36e8ef85c7c..8a19cbba5ab 100644 --- a/tests/subsys/zbus/runtime_observers_registration_alloc_none/src/main.c +++ b/tests/subsys/zbus/runtime_observers_registration_alloc_none/src/main.c @@ -213,4 +213,70 @@ ZTEST(basic, test_specification_based__zbus_obs_priority) zassert_equal(execution_sequence[5], 1); } +/** + * @brief Test removing the first observer in a runtime observer list. + */ +ZTEST(basic, test_remove_first_runtime_observer_alloc_none) +{ + struct sensor_data_msg sd = {.a = 42, .b = 84}; + static struct zbus_observer_node n1, n2, n3; + + /* Reset callback counters */ + count_callback1 = 0; + count_callback2 = 0; + + /* Start with an empty channel (chan3) */ + zassert_equal(0, zbus_chan_pub(&chan3, &sd, K_MSEC(500))); + zassert_equal(count_callback1, 0, "No observers should be called initially"); + zassert_equal(count_callback2, 0, "No observers should be called initially"); + + /* Add multiple observers to create a list using alloc_none variant */ + zassert_equal(0, zbus_chan_add_obs_with_node(&chan3, &lis1, &n1, K_MSEC(200)), + "First observer should be added successfully"); + zassert_equal(0, zbus_chan_add_obs_with_node(&chan3, &lis3, &n2, K_MSEC(200)), + "Second observer should be added successfully"); + zassert_equal(0, zbus_chan_add_obs_with_node(&chan3, &lis4, &n3, K_MSEC(200)), + "Third observer should be added successfully"); + + /* Verify all observers are called */ + zassert_equal(0, zbus_chan_pub(&chan3, &sd, K_MSEC(500))); + zassert_equal(count_callback1, 1, "lis1 callback should be called once"); + zassert_equal(count_callback2, 2, "lis3 and lis4 callbacks should be called"); + + /* Reset counters for the main test */ + count_callback1 = 0; + count_callback2 = 0; + + /* Remove the first observer in the list (lis1) */ + zassert_equal(0, zbus_chan_rm_obs(&chan3, &lis1, K_MSEC(200)), + "First observer should be removed successfully"); + + /* Verify only the remaining observers are called */ + zassert_equal(0, zbus_chan_pub(&chan3, &sd, K_MSEC(500))); + zassert_equal(count_callback1, 0, "lis1 callback should not be called after removal"); + zassert_equal(count_callback2, 2, "lis3 and lis4 callbacks should still be called"); + + /* Remove the new first observer (lis3) to test the same scenario again */ + count_callback2 = 0; + zassert_equal(0, zbus_chan_rm_obs(&chan3, &lis3, K_MSEC(200)), + "New first observer should be removed successfully"); + + /* Verify only lis4 is called */ + zassert_equal(0, zbus_chan_pub(&chan3, &sd, K_MSEC(500))); + zassert_equal(count_callback2, 1, "Only lis4 callback should be called"); + + /* Remove the last observer */ + count_callback2 = 0; + zassert_equal(0, zbus_chan_rm_obs(&chan3, &lis4, K_MSEC(200)), + "Last observer should be removed successfully"); + + /* Verify no observers are called */ + zassert_equal(0, zbus_chan_pub(&chan3, &sd, K_MSEC(500))); + zassert_equal(count_callback2, 0, "No callbacks should be called after all removed"); + + /* Test error case: try to remove non-existent observer */ + zassert_equal(-ENODATA, zbus_chan_rm_obs(&chan3, &lis1, K_MSEC(200)), + "Removing non-existent observer should return -ENODATA"); +} + ZTEST_SUITE(basic, NULL, NULL, NULL, NULL, NULL);