From 563520dd6e750d4d78c1a73df515c23a05e91898 Mon Sep 17 00:00:00 2001 From: Daniel Adam Date: Fri, 27 Sep 2024 19:01:15 +0200 Subject: [PATCH] Fix issues reported by Coverity --- .github/workflows/cmake-linux.yml | 4 +-- .github/workflows/coverity.yml | 1 + api/oc_collection.c | 2 ++ api/oc_endpoint.c | 5 ++++ .../plgd_dps_dhcp.c | 8 +++-- .../device-provisioning-client/plgd_dps_log.c | 5 ++-- .../plgd_dps_provision_cloud.c | 5 +++- .../plgd_dps_retry.c | 6 ++-- api/plgd/unittest/plgd_dps_log.cpp | 19 ------------ apps/dps_cloud_server.c | 29 ++++++++++++++----- apps/secure_mcast_client.c | 9 +++--- port/linux/ip.c | 5 +++- port/linux/tcpsession.c | 11 +++++-- util/jsmn/jsmn.c | 9 ++++-- 14 files changed, 69 insertions(+), 49 deletions(-) diff --git a/.github/workflows/cmake-linux.yml b/.github/workflows/cmake-linux.yml index 471bfa6bf6..2365fd1eea 100644 --- a/.github/workflows/cmake-linux.yml +++ b/.github/workflows/cmake-linux.yml @@ -56,8 +56,8 @@ jobs: - args: "-DOC_IPV4_ENABLED=ON -DOC_TCP_ENABLED=ON -DOC_PKI_ENABLED=OFF" # cloud on (ipv4+tcp on), dynamic allocation off, push notifications off - args: "-DOC_CLOUD_ENABLED=ON -DOC_DYNAMIC_ALLOCATION_ENABLED=OFF -DOC_PUSH_ENABLED=OFF" - # cloud on (ipv4+tcp on), collections create on - - args: "-DOC_CLOUD_ENABLED=ON -DOC_COLLECTIONS_IF_CREATE_ENABLED=ON" + # cloud on (ipv4+tcp on), collections create on, dps on, dps test properties on + - args: "-DOC_CLOUD_ENABLED=ON -DOC_COLLECTIONS_IF_CREATE_ENABLED=ON -DPLGD_DEV_DEVICE_PROVISIONING_ENABLED=ON -DPLGD_DEV_DEVICE_PROVISIONING_TEST_PROPERTIES_ENABLED=ON -DPLGD_DEV_DEVICE_PROVISIONING_MAXIMUM_LOG_LEVEL=INFO" # cloud on (ipv4+tcp on), collections create on, custom message buffer size, custom message buffer pool size, custom app data buffer size, custom app data buffer pool size - args: "-DOC_CLOUD_ENABLED=ON -DOC_COLLECTIONS_IF_CREATE_ENABLED=ON -DOC_INOUT_BUFFER_SIZE=2048 -DOC_INOUT_BUFFER_POOL=4 -DOC_APP_DATA_BUFFER_SIZE=2048 -DOC_APP_DATA_BUFFER_POOL=4" # debug on diff --git a/.github/workflows/coverity.yml b/.github/workflows/coverity.yml index 6e47acbf6c..805f0474d7 100644 --- a/.github/workflows/coverity.yml +++ b/.github/workflows/coverity.yml @@ -29,6 +29,7 @@ jobs: -DPLGD_DEV_TIME_ENABLED=ON -DOC_ETAG_ENABLED=ON -DOC_JSON_ENCODER_ENABLED=ON + -DPLGD_DEV_DEVICE_PROVISIONING_ENABLED=ON -B ${{github.workspace}}/build - uses: vapier/coverity-scan-action@v1 diff --git a/api/oc_collection.c b/api/oc_collection.c index e228411462..f4a91b6312 100644 --- a/api/oc_collection.c +++ b/api/oc_collection.c @@ -295,6 +295,8 @@ oc_get_link_by_uri(oc_collection_t *collection, const char *uri_path, size_t resource_uri_len = oc_string_len(link->resource->uri); while (resource_uri[0] == '/') { resource_uri++; + // overflow check for coverity scan + assert(resource_uri_len > 0); resource_uri_len--; } if (resource_uri_len == uri_path_len && diff --git a/api/oc_endpoint.c b/api/oc_endpoint.c index b42ad6dc68..16ba8ca4a8 100644 --- a/api/oc_endpoint.c +++ b/api/oc_endpoint.c @@ -27,6 +27,9 @@ #include "util/oc_macros_internal.h" #include "util/oc_memb.h" +#include +#include +#include #include #include #include @@ -167,6 +170,8 @@ oc_endpoint_to_cstring(const oc_endpoint_t *endpoint, char *buffer, if (written < 0) { return -1; } + // overflow check for coverity scan + assert(len <= INT_MAX - written && "Integer overflow detected"); return len + written; } diff --git a/api/plgd/device-provisioning-client/plgd_dps_dhcp.c b/api/plgd/device-provisioning-client/plgd_dps_dhcp.c index 6d3c079212..ab69be0633 100644 --- a/api/plgd/device-provisioning-client/plgd_dps_dhcp.c +++ b/api/plgd/device-provisioning-client/plgd_dps_dhcp.c @@ -143,13 +143,15 @@ plgd_dps_hex_string_to_bytes(const char *isc_dhcp_vendor_encapsulated_options, memset(buffer, 0, buffer_size); } for (size_t i = 0; i < isc_dhcp_vendor_encapsulated_options_size;) { + const char *data = isc_dhcp_vendor_encapsulated_options + i; + size_t data_size = isc_dhcp_vendor_encapsulated_options_size - i; uint8_t val = 0; - ssize_t used = - hex_to_value(isc_dhcp_vendor_encapsulated_options + i, - isc_dhcp_vendor_encapsulated_options_size - i, &val); + ssize_t used = hex_to_value(data, data_size, &val); if (used < 0) { return -1; } + // overflow check for coverity scan + assert((size_t)used <= data_size); if (buffer && (needed < buffer_size)) { buffer[needed] = val; } diff --git a/api/plgd/device-provisioning-client/plgd_dps_log.c b/api/plgd/device-provisioning-client/plgd_dps_log.c index 2e18be0247..32c60cfe8a 100644 --- a/api/plgd/device-provisioning-client/plgd_dps_log.c +++ b/api/plgd/device-provisioning-client/plgd_dps_log.c @@ -30,9 +30,8 @@ static struct { - plgd_dps_print_log_fn_t fn; ///< logging function - OC_ATOMIC_INT8_T level; ///< enabled log level - OC_ATOMIC_UINT32_T components; ///< mask of enabled log components + plgd_dps_print_log_fn_t fn; ///< logging function + OC_ATOMIC_INT8_T level; ///< enabled log level } g_dps_logger = { .fn = NULL, .level = OC_LOG_LEVEL_INFO, diff --git a/api/plgd/device-provisioning-client/plgd_dps_provision_cloud.c b/api/plgd/device-provisioning-client/plgd_dps_provision_cloud.c index 6e5025e9f5..e527cefbd9 100644 --- a/api/plgd/device-provisioning-client/plgd_dps_provision_cloud.c +++ b/api/plgd/device-provisioning-client/plgd_dps_provision_cloud.c @@ -181,7 +181,10 @@ dps_handle_set_cloud_response(oc_client_response_t *data) } oc_string_view_t sidv = oc_string_view2(cloud.sid); oc_uuid_t sid; - oc_str_to_uuid_v1(sidv.data, sidv.length, &sid); + if (oc_str_to_uuid_v1(sidv.data, sidv.length, &sid) < 0) { + DPS_ERR("invalid sid(%s) value", sidv.data); + return PLGD_DPS_ERROR_SET_CLOUD; + } const oc_string_t *cloud_apn = oc_cloud_get_authorization_provider_name(cloud_ctx); if (dps_is_equal_string(*cloud_ctx_cis, *cloud.ci_server) && diff --git a/api/plgd/device-provisioning-client/plgd_dps_retry.c b/api/plgd/device-provisioning-client/plgd_dps_retry.c index a016231a37..13c49eddef 100644 --- a/api/plgd/device-provisioning-client/plgd_dps_retry.c +++ b/api/plgd/device-provisioning-client/plgd_dps_retry.c @@ -125,11 +125,11 @@ get_delay_from_timeout(uint16_t timeout) if (timeout == 0) { return oc_random_value() % MIN_DELAYED_VALUE_MS; } - uint64_t delay = (uint64_t)timeout * MILLISECONDS_PER_SECOND / 2; + uint32_t delay = (uint32_t)timeout * MILLISECONDS_PER_SECOND / 2; // Include a random delay to prevent multiple devices from attempting to // connect or make requests simultaneously. - delay += oc_random_value() % delay; - return delay; + uint32_t random_delay = oc_random_value() % delay; + return (uint64_t)delay + random_delay; } static bool diff --git a/api/plgd/unittest/plgd_dps_log.cpp b/api/plgd/unittest/plgd_dps_log.cpp index ec1e8dd8bb..8048b158c3 100644 --- a/api/plgd/unittest/plgd_dps_log.cpp +++ b/api/plgd/unittest/plgd_dps_log.cpp @@ -96,23 +96,4 @@ TEST_F(TestDPSLog, LogToFunction) DPS_LOG(OC_LOG_LEVEL_TRACE, "trace"); } -static void -expectNoLog(oc_log_level_t, const char *, int, const char *, const char *, ...) -{ - FAIL() << "unexpected log"; -} - -TEST_F(TestDPSLog, SkipLogByComponent) -{ - plgd_dps_log_set_level(OC_LOG_LEVEL_TRACE); - plgd_dps_set_log_fn(expectNoLog); - - DPS_ERR("error"); - DPS_WRN("warning"); - DPS_NOTE("notice"); - DPS_INFO("info"); - DPS_DBG("debug"); - DPS_TRACE("trace"); -} - #endif /* OC_HAS_FEATURE_PLGD_DEVICE_PROVISIONING */ diff --git a/apps/dps_cloud_server.c b/apps/dps_cloud_server.c index c57699c299..a0fc9fa68f 100644 --- a/apps/dps_cloud_server.c +++ b/apps/dps_cloud_server.c @@ -853,12 +853,21 @@ register_collection(size_t device) oc_resource_set_discoverable(col, true); oc_resource_set_observable(col, true); - oc_collection_add_supported_rt(col, "oic.r.switch.binary"); - oc_collection_add_mandatory_rt(col, "oic.r.switch.binary"); + if (!oc_collection_add_supported_rt(col, "oic.r.switch.binary")) { + printf("ERROR: could not add supported resource type to collection\n"); + return false; + } + if (!oc_collection_add_mandatory_rt(col, "oic.r.switch.binary")) { + printf("ERROR: could not add mandatory resource type to collection\n"); + return false; + } #ifdef OC_COLLECTIONS_IF_CREATE oc_resource_bind_resource_interface(col, OC_IF_CREATE); - oc_collections_add_rt_factory("oic.r.switch.binary", get_switch_instance, - free_switch_instance); + if (!oc_collections_add_rt_factory("oic.r.switch.binary", get_switch_instance, + free_switch_instance)) { + OC_PRINTF("ERROR: could not register rt factory\n"); + return false; + } #endif /* OC_COLLECTIONS_IF_CREATE */ /* The following enables baseline RETRIEVEs/UPDATEs to Collection properties */ @@ -1508,15 +1517,19 @@ dps_dhcp_parse_vendor_encapsulated_options(const char *value, size_t size, ssize_t len = plgd_dps_hex_string_to_bytes(value, size, NULL, 0); if (len < 0) { printf("ERROR: invalid character in vendor encapsulated options\n"); - return true; + return false; } - if (len > (ssize_t)(sizeof(veo->value))) { + if ((size_t)len > sizeof(veo->value)) { printf("ERROR: vendor encapsulated options too long\n"); - return true; + return false; } len = plgd_dps_hex_string_to_bytes(value, size, veo->value, sizeof(veo->value)); - if (len < (ssize_t)(sizeof(veo->value))) { + if (len < 0) { + printf("ERROR: invalid hex string\n"); + return false; + } + if ((size_t)len < sizeof(veo->value)) { veo->value[len] = '\0'; } veo->size = (size_t)len; diff --git a/apps/secure_mcast_client.c b/apps/secure_mcast_client.c index 1ba6f26561..4c486afe82 100644 --- a/apps/secure_mcast_client.c +++ b/apps/secure_mcast_client.c @@ -330,12 +330,13 @@ discovery(const char *di, const char *uri, oc_string_array_t types, const oc_endpoint_t *ep = endpoint; oc_string_t ep_str; bool supports_mcast = false; - while (ep) { + while (ep != NULL) { memset(&ep_str, 0, sizeof(oc_string_t)); - if (oc_endpoint_to_string(ep, &ep_str) >= 0) { - if ((oc_string_len(ep_str) == 23 && + if (oc_endpoint_to_string(ep, &ep_str) == 0) { + size_t ep_str_len = oc_string_len(ep_str); + if ((ep_str_len == 23 && memcmp(oc_string(ep_str), "coap://224.0.1.187:5683", 23) == 0) || - (oc_string_len(ep_str) == 23 && + (ep_str_len == 23 && memcmp(oc_string(ep_str), "coap://[ff02::158]:5683", 23) == 0)) { supports_mcast = true; } diff --git a/port/linux/ip.c b/port/linux/ip.c index 5c76f6d639..b96668d63d 100644 --- a/port/linux/ip.c +++ b/port/linux/ip.c @@ -28,6 +28,7 @@ #include #include #include +#include #include #include @@ -119,7 +120,9 @@ oc_ip_send_msg(int sock, struct sockaddr_storage *receiver, OC_ERR("sendmsg failed (error %d)", (int)errno); break; } - bytes_sent += ret; + // overflow check for coverity scan + assert(bytes_sent <= SIZE_MAX - (size_t)ret && "Integer overflow detected"); + bytes_sent += (size_t)ret; } OC_TRACE("Sent %zu bytes", bytes_sent); if (bytes_sent == 0) { diff --git a/port/linux/tcpsession.c b/port/linux/tcpsession.c index 6a0f520699..39f9ee9058 100644 --- a/port/linux/tcpsession.c +++ b/port/linux/tcpsession.c @@ -50,6 +50,7 @@ #include #include #include +#include #include #include @@ -939,8 +940,9 @@ tcp_send_message(int sockfd, const oc_message_t *message) { size_t bytes_sent = 0; do { - ssize_t send_len = send(sockfd, message->data + bytes_sent, - message->length - bytes_sent, MSG_NOSIGNAL); + const void *data = message->data + bytes_sent; + size_t data_length = message->length - bytes_sent; + ssize_t send_len = send(sockfd, data, data_length, MSG_NOSIGNAL); if (send_len < 0) { if (errno == EINTR) { continue; @@ -951,7 +953,10 @@ tcp_send_message(int sockfd, const oc_message_t *message) } return (int)bytes_sent; } - bytes_sent += send_len; + // overflow check for coverity scan + assert(bytes_sent <= SIZE_MAX - (size_t)send_len && + "Integer overflow detected"); + bytes_sent += (size_t)send_len; } while (bytes_sent < message->length); OC_TRACE("Sent %zu bytes", bytes_sent); diff --git a/util/jsmn/jsmn.c b/util/jsmn/jsmn.c index f735664540..83e5c89cd9 100644 --- a/util/jsmn/jsmn.c +++ b/util/jsmn/jsmn.c @@ -45,6 +45,7 @@ #include #include +#include #include #include @@ -247,6 +248,8 @@ jsmn_parse_next_char(jsmn_parser_t *parser, jsmntok_t *token, const char *js, if (r < 0) { return r; } + // overflow check for coverity scan + assert(count <= INT_MAX - r && "Integer overflow detected"); count += r; break; } @@ -289,12 +292,14 @@ jsmn_parse(jsmn_parser_t *parser, const char *js, const size_t len, { jsmntok_t token; jsmn_init_token(&token); - unsigned count = 0; + int count = 0; for (; parser->pos < len && js[parser->pos] != '\0'; parser->pos++) { int r = jsmn_parse_next_char(parser, &token, js, len, cb, data); if (r < 0) { return r; } + // overflow check for coverity scan + assert(count <= INT_MAX - r && "Integer overflow detected"); count += r; } @@ -302,7 +307,7 @@ jsmn_parse(jsmn_parser_t *parser, const char *js, const size_t len, return JSMN_ERROR_PART; } - return (int)count; + return count; } void