From a5bfa62ccadb5ed1546cdf848140c88edcbda829 Mon Sep 17 00:00:00 2001 From: Daniel Adam Date: Sat, 8 Jul 2023 12:40:08 +0200 Subject: [PATCH] fixup! messaging: fix coverity issues --- .github/workflows/sonar-cloud-analysis.yml | 2 +- api/oc_ri.c | 2 +- messaging/coap/observe.c | 150 +++++++++++---------- messaging/coap/observe.h | 12 +- messaging/coap/unittest/observetest.cpp | 54 ++++++++ 5 files changed, 142 insertions(+), 78 deletions(-) diff --git a/.github/workflows/sonar-cloud-analysis.yml b/.github/workflows/sonar-cloud-analysis.yml index ca4c26cddf..e8c89ac4a6 100644 --- a/.github/workflows/sonar-cloud-analysis.yml +++ b/.github/workflows/sonar-cloud-analysis.yml @@ -26,7 +26,7 @@ jobs: matrix: include: # cloud (ipv4+tcp) on, collection create on, push on, rfotm on - - build_args: "-DOC_CLOUD_ENABLED=ON -DOC_COLLECTIONS_IF_CREATE_ENABLED=ON -DOC_PUSH_ENABLED=ON -DOC_RESOURCE_ACCESS_IN_RFOTM_ENABLED=ON" + - build_args: "-DOC_CLOUD_ENABLED=ON -DOC_COLLECTIONS_IF_CREATE_ENABLED=ON -DOC_PUSH_ENABLED=ON -DOC_DISCOVERY_RESOURCE_OBSERVABLE_ENABLED=ON -DOC_RESOURCE_ACCESS_IN_RFOTM_ENABLED=ON" # ipv6 dns on, oscore off, rep realloc on - build_args: "-DOC_DNS_LOOKUP_IPV6_ENABLED=ON -DOC_OSCORE_ENABLED=OFF -DOC_REP_ENCODING_REALLOC=ON" # ipv4 on, tcp on, dynamic allocation off, rfotm on diff --git a/api/oc_ri.c b/api/oc_ri.c index b64284ea1e..f0d0e0eb87 100644 --- a/api/oc_ri.c +++ b/api/oc_ri.c @@ -1087,7 +1087,7 @@ oc_observe_notification_resource_defaults_delayed(void *data) oc_resource_defaults_data_t *resource_defaults_data = (oc_resource_defaults_data_t *)data; notify_resource_defaults_observer(resource_defaults_data->resource, - resource_defaults_data->iface_mask, NULL); + resource_defaults_data->iface_mask); oc_ri_dealloc_resource_defaults(resource_defaults_data); return OC_EVENT_DONE; } diff --git a/messaging/coap/observe.c b/messaging/coap/observe.c index a6b35bff6b..e7672181be 100644 --- a/messaging/coap/observe.c +++ b/messaging/coap/observe.c @@ -101,8 +101,8 @@ typedef struct batch_observer oc_string_t removed_resource_uri; } batch_observer_t; -OC_LIST(oc_batch_observers_list); -OC_MEMB(oc_batch_observers_memb, batch_observer_t, COAP_MAX_OBSERVERS); +OC_LIST(g_batch_observers_list); +OC_MEMB(g_batch_observers_memb, batch_observer_t, COAP_MAX_OBSERVERS); typedef bool cmp_batch_observer_t(batch_observer_t *o, void *ctx); @@ -138,19 +138,19 @@ free_batch_observer(batch_observer_t *batch_obs) return; } oc_free_string(&batch_obs->removed_resource_uri); - oc_memb_free(&oc_batch_observers_memb, batch_obs); + oc_memb_free(&g_batch_observers_memb, batch_obs); } static void remove_discovery_batch_observers(cmp_batch_observer_t *cmp, void *ctx) { batch_observer_t *batch_obs = - (batch_observer_t *)oc_list_head(oc_batch_observers_list); + (batch_observer_t *)oc_list_head(g_batch_observers_list); while (batch_obs != NULL) { if (cmp(batch_obs, ctx)) { - oc_list_remove(oc_batch_observers_list, batch_obs); + oc_list_remove(g_batch_observers_list, batch_obs); free_batch_observer(batch_obs); - batch_obs = (batch_observer_t *)oc_list_head(oc_batch_observers_list); + batch_obs = (batch_observer_t *)oc_list_head(g_batch_observers_list); } else { batch_obs = batch_obs->next; } @@ -171,8 +171,8 @@ observe_increment_observe_counter(int32_t *counter) return prev; } -OC_LIST(oc_observers_list); -OC_MEMB(oc_observers_memb, coap_observer_t, COAP_MAX_OBSERVERS); +OC_LIST(g_observers_list); +OC_MEMB(g_observers_memb, coap_observer_t, COAP_MAX_OBSERVERS); /*---------------------------------------------------------------------------*/ /*- Internal API ------------------------------------------------------------*/ @@ -181,7 +181,7 @@ OC_MEMB(oc_observers_memb, coap_observer_t, COAP_MAX_OBSERVERS); oc_list_t coap_get_observers(void) { - return oc_observers_list; + return g_observers_list; } // TODO: use oc_string_view_t @@ -233,11 +233,11 @@ coap_remove_observer(coap_observer_t *o) #endif /* OC_BLOCK_WISE */ o->resource->num_observers--; oc_free_string(&o->url); - oc_list_remove(oc_observers_list, o); + oc_list_remove(g_observers_list, o); #if defined(OC_RES_BATCH_SUPPORT) && defined(OC_DISCOVERY_RESOURCE_OBSERVABLE) remove_discovery_batch_observers(cmp_batch_by_observer, o); #endif /* OC_RES_BATCH_SUPPORT && OC_DISCOVERY_RESOURCE_OBSERVABLE */ - oc_memb_free(&oc_observers_memb, o); + oc_memb_free(&g_observers_memb, o); } typedef void (*coap_on_remove_observer_handle_fn_t)(const coap_observer_t *obs); @@ -251,7 +251,7 @@ coap_remove_observers_by_filter(coap_on_remove_observer_filter_t filter, bool match_all) { int removed = 0; - coap_observer_t *obs = (coap_observer_t *)oc_list_head(oc_observers_list); + coap_observer_t *obs = (coap_observer_t *)oc_list_head(g_observers_list); while (obs != NULL) { coap_observer_t *next = obs->next; if (filter(obs, filter_data)) { @@ -311,7 +311,7 @@ coap_add_observer(oc_resource_t *resource, uint16_t block2_size, /* Remove existing observe relationship, if any. */ int dup = coap_remove_observer_duplicates(endpoint, uri, uri_len, iface_mask); - coap_observer_t *o = oc_memb_alloc(&oc_observers_memb); + coap_observer_t *o = oc_memb_alloc(&g_observers_memb); if (o == NULL) { OC_WRN("insufficient memory to add new observer"); return NULL; @@ -332,23 +332,23 @@ coap_add_observer(oc_resource_t *resource, uint16_t block2_size, resource->num_observers++; #ifdef OC_DYNAMIC_ALLOCATION OC_DBG("Adding observer (%u) for /%s [0x%02X%02X]", - oc_list_length(oc_observers_list) + 1, oc_string(o->url), o->token[0], + oc_list_length(g_observers_list) + 1, oc_string(o->url), o->token[0], o->token[1]); #else /* OC_DYNAMIC_ALLOCATION */ OC_DBG("Adding observer (%u/%u) for /%s [0x%02X%02X]", - oc_list_length(oc_observers_list) + 1, COAP_MAX_OBSERVERS, + oc_list_length(g_observers_list) + 1, COAP_MAX_OBSERVERS, oc_string(o->url), o->token[0], o->token[1]); #endif /* !OC_DYNAMIC_ALLOCATION */ OC_DBG("Removed %d duplicate observer(s)", dup); (void)dup; - oc_list_add(oc_observers_list, o); + oc_list_add(g_observers_list, o); return o; } void coap_free_all_observers(void) { - coap_observer_t *obs = (coap_observer_t *)oc_list_head(oc_observers_list); + coap_observer_t *obs = (coap_observer_t *)oc_list_head(g_observers_list); while (obs != NULL) { coap_observer_t *next = obs->next; coap_remove_observer(obs); @@ -508,6 +508,43 @@ coap_remove_observers_by_resource(const oc_resource_t *rsc) return removed; } +#ifdef OC_SECURITY + +typedef struct device_with_dos_change_t +{ + size_t device; + bool reset; +} device_with_dos_change_t; + +static bool +coap_observer_has_matching_device(const coap_observer_t *obs, const void *data) +{ + const device_with_dos_change_t *ddc = (const device_with_dos_change_t *)data; + return obs->endpoint.device == ddc->device && + (ddc->reset || + !oc_sec_check_acl(OC_GET, obs->resource, &obs->endpoint)); +} + +static void +send_service_unavailable_notification(const coap_observer_t *obs) +{ + send_cancellation_notification(obs, SERVICE_UNAVAILABLE_5_03); +} + +int +coap_remove_observers_on_dos_change(size_t device, bool reset) +{ + OC_DBG("Unregistering observers for device %zd (reset=%d)", device, + (int)reset); + device_with_dos_change_t ddc = { device, reset }; + int removed = coap_remove_observers_by_filter( + coap_observer_has_matching_device, &ddc, + send_service_unavailable_notification, true); + OC_DBG("Removed %d observers", removed); + return removed; +} +#endif /* OC_SECURITY */ + /*---------------------------------------------------------------------------*/ /*- Notification ------------------------------------------------------------*/ /*---------------------------------------------------------------------------*/ @@ -591,7 +628,7 @@ coap_prepare_notification_blockwise(coap_packet_t *notification, coap_set_payload(notification, payload, payload_size); coap_set_header_block2(notification, 0, 1, obs->block2_size); coap_set_header_size2(notification, response_state->payload_size); - oc_blockwise_response_state_t *bwt_res_state = + const oc_blockwise_response_state_t *bwt_res_state = (oc_blockwise_response_state_t *)response_state; coap_set_header_etag(notification, bwt_res_state->etag, COAP_ETAG_LEN); } @@ -714,8 +751,7 @@ coap_notify_collection_observers(const oc_collection_t *collection, oc_response_t response = { 0 }; response.response_buffer = response_buf; /* iterate over observers */ - for (coap_observer_t *obs = - (coap_observer_t *)oc_list_head(oc_observers_list); + for (coap_observer_t *obs = (coap_observer_t *)oc_list_head(g_observers_list); obs; obs = obs->next) { if (obs->resource != (const oc_resource_t *)collection) { continue; @@ -813,7 +849,7 @@ coap_notify_collection_links_list(oc_collection_t *collection) } static int -coap_notify_collections(oc_resource_t *resource) +coap_notify_collections(const oc_resource_t *resource) { #ifndef OC_DYNAMIC_ALLOCATION uint8_t buffer[OC_MIN_OBSERVE_SIZE]; @@ -871,25 +907,6 @@ coap_notify_collections(oc_resource_t *resource) } #endif /* OC_COLLECTIONS */ -#ifdef OC_SECURITY -int -coap_remove_observers_on_dos_change(size_t device, bool reset) -{ - coap_observer_t *obs = (coap_observer_t *)oc_list_head(oc_observers_list); - /* iterate over observers */ - while (obs) { - coap_observer_t *next = obs->next; - if (obs->endpoint.device == device && - (reset || !oc_sec_check_acl(OC_GET, obs->resource, &obs->endpoint))) { - send_cancellation_notification(obs, SERVICE_UNAVAILABLE_5_03); - coap_remove_observer(obs); - } - obs = next; - } - return 0; -} -#endif /* OC_SECURITY */ - static bool fill_response(oc_resource_t *resource, const oc_endpoint_t *endpoint, oc_interface_mask_t iface_mask, oc_response_t *response) @@ -963,7 +980,7 @@ coap_notify_observers_internal(oc_resource_t *resource, #else /* !OC_DYNAMIC_ALLOCATION */ uint8_t *buffer = malloc(OC_MIN_OBSERVE_SIZE); if (!buffer) { - OC_WRN("coap_notify_observers: out of memory allocating buffer"); + OC_WRN("coap_notify_observers_internal: out of memory allocating buffer"); return 0; } //! buffer #endif /* OC_DYNAMIC_ALLOCATION */ @@ -991,11 +1008,11 @@ coap_notify_observers_internal(oc_resource_t *resource, } } //! response_buf && resource - oc_resource_t *discover_resource = + const oc_resource_t *discover_resource = oc_core_get_resource_by_index(OCF_RES, resource->device); /* iterate over observers */ for (coap_observer_t *obs = - (coap_observer_t *)oc_list_head(oc_observers_list); + (coap_observer_t *)oc_list_head(g_observers_list); obs; obs = obs->next) { if ((obs->resource != resource) || (endpoint && oc_endpoint_compare(&obs->endpoint, endpoint) != 0)) { @@ -1063,16 +1080,15 @@ coap_notify_observers_internal(oc_resource_t *resource, void notify_resource_defaults_observer(oc_resource_t *resource, - oc_interface_mask_t iface_mask, - oc_response_buffer_t *response_buf) + oc_interface_mask_t iface_mask) { - (void)response_buf; #ifndef OC_DYNAMIC_ALLOCATION uint8_t buffer[OC_MIN_OBSERVE_SIZE]; #else /* !OC_DYNAMIC_ALLOCATION */ uint8_t *buffer = malloc(OC_MIN_OBSERVE_SIZE); if (!buffer) { - OC_WRN("coap_notify_observers: out of memory allocating buffer"); + OC_WRN( + "notify_resource_defaults_observer: out of memory allocating buffer"); return; } //! buffer #endif /* OC_DYNAMIC_ALLOCATION */ @@ -1084,8 +1100,7 @@ notify_resource_defaults_observer(oc_resource_t *resource, response_buffer.buffer_size = OC_MIN_OBSERVE_SIZE; response.response_buffer = &response_buffer; /* iterate over observers */ - for (coap_observer_t *obs = - (coap_observer_t *)oc_list_head(oc_observers_list); + for (coap_observer_t *obs = (coap_observer_t *)oc_list_head(g_observers_list); obs; obs = obs->next) { if (obs->resource != resource) { continue; @@ -1160,7 +1175,7 @@ process_batch_observers(void *data) { (void)data; batch_observer_t *batch_obs = - (batch_observer_t *)oc_list_head(oc_batch_observers_list); + (batch_observer_t *)oc_list_head(g_batch_observers_list); if (batch_obs == NULL) { return OC_EVENT_DONE; } @@ -1185,9 +1200,9 @@ process_batch_observers(void *data) !oc_string_len(batch_obs->removed_resource_uri)) { OC_WRN("process_batch_observers: resource is NULL and " "removed_resource_uri is empty"); - oc_list_remove(oc_batch_observers_list, batch_obs); + oc_list_remove(g_batch_observers_list, batch_obs); free_batch_observer(batch_obs); - batch_obs = (batch_observer_t *)oc_list_head(oc_batch_observers_list); + batch_obs = (batch_observer_t *)oc_list_head(g_batch_observers_list); continue; } coap_observer_t *obs = batch_obs->obs; @@ -1216,7 +1231,7 @@ process_batch_observers(void *data) batch_observer_t *next = o->next; if (o->obs == obs) { create_batch_for_batch_observer(&links_array, o, &obs->endpoint); - oc_list_remove(oc_batch_observers_list, o); + oc_list_remove(g_batch_observers_list, o); free_batch_observer(o); } o = next; @@ -1247,9 +1262,9 @@ process_batch_observers(void *data) #ifdef OC_DYNAMIC_ALLOCATION response_buffer.buffer_size = oc_rep_get_encoder_buffer_size(); #endif /* OC_DYNAMIC_ALLOCATION */ - oc_list_remove(oc_batch_observers_list, batch_obs); + oc_list_remove(g_batch_observers_list, batch_obs); free_batch_observer(batch_obs); - batch_obs = (batch_observer_t *)oc_list_head(oc_batch_observers_list); + batch_obs = (batch_observer_t *)oc_list_head(g_batch_observers_list); } leave_notify_observers: #ifdef OC_DYNAMIC_ALLOCATION @@ -1297,8 +1312,7 @@ add_notification_batch_observers_list(oc_resource_t *resource, bool removed) } /* iterate over observers */ - for (coap_observer_t *obs = - (coap_observer_t *)oc_list_head(oc_observers_list); + for (coap_observer_t *obs = (coap_observer_t *)oc_list_head(g_observers_list); obs; obs = obs->next) { if (obs->resource != discover_resource || obs->iface_mask != OC_IF_B) { continue; @@ -1311,7 +1325,7 @@ add_notification_batch_observers_list(oc_resource_t *resource, bool removed) // deduplicate observations. bool found = false; batch_observer_t *batch_obs = NULL; - for (batch_obs = (batch_observer_t *)oc_list_head(oc_batch_observers_list); + for (batch_obs = (batch_observer_t *)oc_list_head(g_batch_observers_list); batch_obs; batch_obs = batch_obs->next) { if (cmp_add_batch_observer_resource(batch_obs, obs, resource, removed)) { found = true; @@ -1321,7 +1335,7 @@ add_notification_batch_observers_list(oc_resource_t *resource, bool removed) if (found) { continue; } - batch_observer_t *o = oc_memb_alloc(&oc_batch_observers_memb); + batch_observer_t *o = oc_memb_alloc(&g_batch_observers_memb); if (o == NULL) { OC_ERR("coap_notify_discovery_batch_observers: cannot allocate batch " "observer for resource %s", @@ -1335,7 +1349,7 @@ add_notification_batch_observers_list(oc_resource_t *resource, bool removed) } else { o->resource = resource; } - oc_list_add(oc_batch_observers_list, o); + oc_list_add(g_batch_observers_list, o); } } dispatch_process_batch_observers(); @@ -1380,8 +1394,7 @@ notify_discovery_observers(oc_resource_t *resource) response.response_buffer = &response_buffer; /* iterate over observers */ - for (coap_observer_t *obs = - (coap_observer_t *)oc_list_head(oc_observers_list); + for (coap_observer_t *obs = (coap_observer_t *)oc_list_head(g_observers_list); obs; obs = obs->next) { if (obs->resource != resource) { continue; @@ -1413,7 +1426,6 @@ coap_notify_observers(oc_resource_t *resource, oc_response_buffer_t *response_buf, const oc_endpoint_t *endpoint) { - int num = 0; #ifdef OC_DISCOVERY_RESOURCE_OBSERVABLE #ifdef OC_RES_BATCH_SUPPORT coap_notify_discovery_batch_observers(resource); @@ -1421,13 +1433,10 @@ coap_notify_observers(oc_resource_t *resource, oc_resource_t *discover_resource = oc_core_get_resource_by_index(OCF_RES, resource->device); if (resource == discover_resource) { - num = notify_discovery_observers(discover_resource); - } else -#endif /* OC_DISCOVERY_RESOURCE_OBSERVABLE */ - { - num = coap_notify_observers_internal(resource, response_buf, endpoint); + return notify_discovery_observers(discover_resource); } - return num; +#endif /* OC_DISCOVERY_RESOURCE_OBSERVABLE */ + return coap_notify_observers_internal(resource, response_buf, endpoint); } int @@ -1464,8 +1473,7 @@ coap_want_be_notified(const oc_resource_t *resource) oc_core_get_resource_by_index(OCF_RES, resource->device); #endif /* OC_DISCOVERY_RESOURCE_OBSERVABLE && OC_RES_BATCH_SUPPORT */ /* iterate over observers */ - for (coap_observer_t *obs = - (coap_observer_t *)oc_list_head(oc_observers_list); + for (coap_observer_t *obs = (coap_observer_t *)oc_list_head(g_observers_list); obs; obs = obs->next) { if (obs->resource == resource) { return true; diff --git a/messaging/coap/observe.h b/messaging/coap/observe.h index 9a0ecbd6eb..5feee4612d 100644 --- a/messaging/coap/observe.h +++ b/messaging/coap/observe.h @@ -140,16 +140,20 @@ bool coap_remove_observer_by_mid(const oc_endpoint_t *endpoint, uint16_t mid) */ int coap_remove_observers_by_resource(const oc_resource_t *rsc) OC_NONNULL(); -void coap_remove_discovery_batch_observers_by_resource(oc_resource_t *resource); +/** @brief Deallocate and remove all observers on DOS change */ +int coap_remove_observers_on_dos_change(size_t device, bool reset); + +/** @brief Deallocate and remove all observers. */ void coap_free_all_observers(void); + +void coap_remove_discovery_batch_observers_by_resource(oc_resource_t *resource); void coap_notify_discovery_batch_observers(oc_resource_t *resource); int coap_notify_observers(oc_resource_t *resource, oc_response_buffer_t *response_buf, const oc_endpoint_t *endpoint); bool coap_want_be_notified(const oc_resource_t *resource); void notify_resource_defaults_observer(oc_resource_t *resource, - oc_interface_mask_t iface_mask, - oc_response_buffer_t *response_buf); + oc_interface_mask_t iface_mask); /** * @brief Construct observation response with OC_IF_LL interface and send it to @@ -199,8 +203,6 @@ int coap_observe_handler(const coap_packet_t *request, uint16_t block2_size, const oc_endpoint_t *endpoint, oc_interface_mask_t iface_mask) OC_NONNULL(); -int coap_remove_observers_on_dos_change(size_t device, bool reset); - #ifdef __cplusplus } #endif diff --git a/messaging/coap/unittest/observetest.cpp b/messaging/coap/unittest/observetest.cpp index c2424729a8..12c5ab2d54 100644 --- a/messaging/coap/unittest/observetest.cpp +++ b/messaging/coap/unittest/observetest.cpp @@ -19,9 +19,11 @@ #include "api/oc_helpers_internal.h" #include "messaging/coap/observe.h" #include "messaging/coap/transactions.h" +#include "oc_core_res.h" #include "oc_ri.h" #include "port/oc_network_event_handler_internal.h" #include "port/oc_random.h" +#include "tests/gtest/Device.h" #include "tests/gtest/Endpoint.h" #include "util/oc_list.h" @@ -441,3 +443,55 @@ TEST_F(TestObserver, RemoveAllObserversByResource) // must be called here so pointers to local variables are still valid coap_free_all_observers(); } + +#ifdef OC_SECURITY + +static constexpr size_t kDeviceID{ 0 }; + +class TestObserverWithServer : public testing::Test { +public: + static void SetUpTestCase() { ASSERT_TRUE(oc::TestDevice::StartServer()); } + + static void TearDownTestCase() { oc::TestDevice::StopServer(); } +}; + +TEST_F(TestObserverWithServer, RemoveAllObserversOnDOSChange) +{ + const oc_endpoint_t *ep = oc::TestDevice::GetEndpoint(kDeviceID, 0, SECURED); + ASSERT_NE(nullptr, ep); + + std::array token; + oc_random_buffer(token.data(), token.size()); + + oc_resource_t *platform = oc_core_get_resource_by_index(OCF_P, kDeviceID); + ASSERT_NE(nullptr, platform); + EXPECT_NE(nullptr, + coap_add_observer(platform, 1024, ep, token.data(), token.size(), + oc_string(platform->uri), + oc_string_len(platform->uri), OC_IF_BASELINE)); + + oc_resource_t *con = oc_core_get_resource_by_index(OCF_CON, kDeviceID); + ASSERT_NE(nullptr, con); + EXPECT_NE(nullptr, + coap_add_observer(con, 1024, ep, token.data(), token.size(), + oc_string(con->uri), oc_string_len(con->uri), + OC_IF_BASELINE)); + + oc_resource_t *doxm = oc_core_get_resource_by_index(OCF_SEC_DOXM, kDeviceID); + ASSERT_NE(nullptr, doxm); + EXPECT_NE(nullptr, + coap_add_observer(doxm, 1024, ep, token.data(), token.size(), + oc_string(doxm->uri), oc_string_len(doxm->uri), + OC_IF_BASELINE)); + + size_t kInvalidDeviceID = std::numeric_limits::max(); + EXPECT_EQ(0, coap_remove_observers_on_dos_change(kInvalidDeviceID, false)); + + // con observer removed + EXPECT_EQ(1, coap_remove_observers_on_dos_change(kDeviceID, false)); + + // platform and doxm observers removed + EXPECT_EQ(2, coap_remove_observers_on_dos_change(kDeviceID, true)); +} + +#endif // OC_SECURITY