Commit a7ce6055 authored by Jan Tore Guggedal's avatar Jan Tore Guggedal Committed by Daniel DeGrasse
Browse files

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: default avatarJan Tore Guggedal <jantore.guggedal@nordicsemi.no>
parent 26818ee1

subsys/zbus/zbus_runtime_observers.c

+3 −1
Original line number Diff line number Diff line
@@ -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

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

+65 −0
Original line number Diff line number Diff line
@@ -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);

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

+66 −0
Original line number Diff line number Diff line
@@ -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);

CRA Git | Maintained and supported by SUSTech CRA and CCSE