Skip to content

Commit a7ce605

Browse files
jtguggedaldanieldegrasse
jtguggedal
authored and
danieldegrasse
committed
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 <[email protected]>
1 parent 26818ee commit a7ce605

File tree

3 files changed

+134
-1
lines changed

3 files changed

+134
-1
lines changed

‎subsys/zbus/zbus_runtime_observers.c

Lines changed: 3 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -175,7 +175,9 @@ int zbus_chan_rm_obs(const struct zbus_channel *chan, const struct zbus_observer
175175

176176
SYS_SLIST_FOR_EACH_CONTAINER_SAFE(&chan->data->observers, obs_nd, tmp, node) {
177177
if (obs_nd->obs == obs) {
178-
sys_slist_remove(&chan->data->observers, &prev_obs_nd->node, &obs_nd->node);
178+
sys_slist_remove(&chan->data->observers,
179+
prev_obs_nd ? &prev_obs_nd->node : NULL,
180+
&obs_nd->node);
179181
#if defined(CONFIG_ZBUS_RUNTIME_OBSERVERS_NODE_ALLOC_NONE)
180182
obs_nd->chan = NULL;
181183
#else

‎tests/subsys/zbus/runtime_observers_registration/src/main.c

Lines changed: 65 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -210,4 +210,69 @@ ZTEST(basic, test_specification_based__zbus_obs_priority)
210210
zassert_equal(execution_sequence[5], 1);
211211
}
212212

213+
/**
214+
* @brief Test removing the first observer in a runtime observer list.
215+
*/
216+
ZTEST(basic, test_remove_first_runtime_observer)
217+
{
218+
struct sensor_data_msg sd = {.a = 42, .b = 84};
219+
220+
/* Reset callback counters */
221+
count_callback1 = 0;
222+
count_callback2 = 0;
223+
224+
/* Start with an empty channel (chan3) */
225+
zassert_equal(0, zbus_chan_pub(&chan3, &sd, K_MSEC(500)));
226+
zassert_equal(count_callback1, 0, "No observers should be called initially");
227+
zassert_equal(count_callback2, 0, "No observers should be called initially");
228+
229+
/* Add multiple observers to create a list */
230+
zassert_equal(0, zbus_chan_add_obs(&chan3, &lis1, K_MSEC(200)),
231+
"First observer should be added successfully");
232+
zassert_equal(0, zbus_chan_add_obs(&chan3, &lis3, K_MSEC(200)),
233+
"Second observer should be added successfully");
234+
zassert_equal(0, zbus_chan_add_obs(&chan3, &lis4, K_MSEC(200)),
235+
"Third observer should be added successfully");
236+
237+
/* Verify all observers are called */
238+
zassert_equal(0, zbus_chan_pub(&chan3, &sd, K_MSEC(500)));
239+
zassert_equal(count_callback1, 1, "lis1 callback should be called once");
240+
zassert_equal(count_callback2, 2, "lis3 and lis4 callbacks should be called");
241+
242+
/* Reset counters for the main test */
243+
count_callback1 = 0;
244+
count_callback2 = 0;
245+
246+
/* Remove the first observer in the list (lis1) */
247+
zassert_equal(0, zbus_chan_rm_obs(&chan3, &lis1, K_MSEC(200)),
248+
"First observer should be removed successfully");
249+
250+
/* Verify only the remaining observers are called */
251+
zassert_equal(0, zbus_chan_pub(&chan3, &sd, K_MSEC(500)));
252+
zassert_equal(count_callback1, 0, "lis1 callback should not be called after removal");
253+
zassert_equal(count_callback2, 2, "lis3 and lis4 callbacks should still be called");
254+
255+
/* Remove the new first observer (lis3) to test the same scenario again */
256+
count_callback2 = 0;
257+
zassert_equal(0, zbus_chan_rm_obs(&chan3, &lis3, K_MSEC(200)),
258+
"New first observer should be removed successfully");
259+
260+
/* Verify only lis4 is called */
261+
zassert_equal(0, zbus_chan_pub(&chan3, &sd, K_MSEC(500)));
262+
zassert_equal(count_callback2, 1, "Only lis4 callback should be called");
263+
264+
/* Remove the last observer */
265+
count_callback2 = 0;
266+
zassert_equal(0, zbus_chan_rm_obs(&chan3, &lis4, K_MSEC(200)),
267+
"Last observer should be removed successfully");
268+
269+
/* Verify no observers are called */
270+
zassert_equal(0, zbus_chan_pub(&chan3, &sd, K_MSEC(500)));
271+
zassert_equal(count_callback2, 0, "No callbacks should be called after all removed");
272+
273+
/* Test error case: try to remove non-existent observer */
274+
zassert_equal(-ENODATA, zbus_chan_rm_obs(&chan3, &lis1, K_MSEC(200)),
275+
"Removing non-existent observer should return -ENODATA");
276+
}
277+
213278
ZTEST_SUITE(basic, NULL, NULL, NULL, NULL, NULL);

‎tests/subsys/zbus/runtime_observers_registration_alloc_none/src/main.c

Lines changed: 66 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -213,4 +213,70 @@ ZTEST(basic, test_specification_based__zbus_obs_priority)
213213
zassert_equal(execution_sequence[5], 1);
214214
}
215215

216+
/**
217+
* @brief Test removing the first observer in a runtime observer list.
218+
*/
219+
ZTEST(basic, test_remove_first_runtime_observer_alloc_none)
220+
{
221+
struct sensor_data_msg sd = {.a = 42, .b = 84};
222+
static struct zbus_observer_node n1, n2, n3;
223+
224+
/* Reset callback counters */
225+
count_callback1 = 0;
226+
count_callback2 = 0;
227+
228+
/* Start with an empty channel (chan3) */
229+
zassert_equal(0, zbus_chan_pub(&chan3, &sd, K_MSEC(500)));
230+
zassert_equal(count_callback1, 0, "No observers should be called initially");
231+
zassert_equal(count_callback2, 0, "No observers should be called initially");
232+
233+
/* Add multiple observers to create a list using alloc_none variant */
234+
zassert_equal(0, zbus_chan_add_obs_with_node(&chan3, &lis1, &n1, K_MSEC(200)),
235+
"First observer should be added successfully");
236+
zassert_equal(0, zbus_chan_add_obs_with_node(&chan3, &lis3, &n2, K_MSEC(200)),
237+
"Second observer should be added successfully");
238+
zassert_equal(0, zbus_chan_add_obs_with_node(&chan3, &lis4, &n3, K_MSEC(200)),
239+
"Third observer should be added successfully");
240+
241+
/* Verify all observers are called */
242+
zassert_equal(0, zbus_chan_pub(&chan3, &sd, K_MSEC(500)));
243+
zassert_equal(count_callback1, 1, "lis1 callback should be called once");
244+
zassert_equal(count_callback2, 2, "lis3 and lis4 callbacks should be called");
245+
246+
/* Reset counters for the main test */
247+
count_callback1 = 0;
248+
count_callback2 = 0;
249+
250+
/* Remove the first observer in the list (lis1) */
251+
zassert_equal(0, zbus_chan_rm_obs(&chan3, &lis1, K_MSEC(200)),
252+
"First observer should be removed successfully");
253+
254+
/* Verify only the remaining observers are called */
255+
zassert_equal(0, zbus_chan_pub(&chan3, &sd, K_MSEC(500)));
256+
zassert_equal(count_callback1, 0, "lis1 callback should not be called after removal");
257+
zassert_equal(count_callback2, 2, "lis3 and lis4 callbacks should still be called");
258+
259+
/* Remove the new first observer (lis3) to test the same scenario again */
260+
count_callback2 = 0;
261+
zassert_equal(0, zbus_chan_rm_obs(&chan3, &lis3, K_MSEC(200)),
262+
"New first observer should be removed successfully");
263+
264+
/* Verify only lis4 is called */
265+
zassert_equal(0, zbus_chan_pub(&chan3, &sd, K_MSEC(500)));
266+
zassert_equal(count_callback2, 1, "Only lis4 callback should be called");
267+
268+
/* Remove the last observer */
269+
count_callback2 = 0;
270+
zassert_equal(0, zbus_chan_rm_obs(&chan3, &lis4, K_MSEC(200)),
271+
"Last observer should be removed successfully");
272+
273+
/* Verify no observers are called */
274+
zassert_equal(0, zbus_chan_pub(&chan3, &sd, K_MSEC(500)));
275+
zassert_equal(count_callback2, 0, "No callbacks should be called after all removed");
276+
277+
/* Test error case: try to remove non-existent observer */
278+
zassert_equal(-ENODATA, zbus_chan_rm_obs(&chan3, &lis1, K_MSEC(200)),
279+
"Removing non-existent observer should return -ENODATA");
280+
}
281+
216282
ZTEST_SUITE(basic, NULL, NULL, NULL, NULL, NULL);

0 commit comments

Comments
 (0)