Fix coverity issues #655
Fix coverity issues #655
Conversation
WalkthroughThe changes primarily refactor string handling in test and implementation code, replacing Changes
Sequence Diagram(s)sequenceDiagram
participant TestCase
participant Resource/Collection API
participant C API
TestCase->>Resource/Collection API: Create resource/collection (pass std::string)
Resource/Collection API->>C API: Call with .c_str() for URI/RT
C API-->>Resource/Collection API: Resource/collection created
Resource/Collection API-->>TestCase: Resource/collection object returned
sequenceDiagram
participant StorageAPI
participant storage_write_path (helper)
participant OS/FileSystem
StorageAPI->>storage_write_path: Validate and concatenate store path
storage_write_path-->>StorageAPI: Success/Error
StorageAPI->>OS/FileSystem: Open/write file with full path
Suggested reviewers
📜 Recent review detailsConfiguration used: CodeRabbit UI ⛔ Files ignored due to path filters (1)
📒 Files selected for processing (13)
🚧 Files skipped from review as they are similar to previous changes (13)
✨ Finishing Touches
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
|
🎉 Thank you for your code contribution! To guarantee the change/addition is conformant to the OCF Specification, we would like to ask you to execute OCF Conformance Testing of your change ☝️ when your work is ready to be reviewed. ℹ️ To verify your latest change (f377812), label this PR with |
Danielius1922
force-pushed
the
adam/feature/654-coverity-fixes-1
branch
2 times, most recently
from
fffd614 to
392ecc9
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
docker/apps/Dockerfile.cloud-server-debug-clang(1 hunks)docker/apps/Dockerfile.dps-cloud-server(2 hunks)tools/collect-coverage.sh(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- docker/apps/Dockerfile.dps-cloud-server
- docker/apps/Dockerfile.cloud-server-debug-clang
⏰ Context from checks skipped due to timeout of 90000ms (40)
- GitHub Check: build-and-push-images (cloud-server, Release, docker/apps/Dockerfile.cloud-server) / build-and-push-image
- GitHub Check: build-and-push-dps-images (dps-cloud-server-debug, Debug, -DOC_DEBUG_ENABLED=ON -DOC_LOG_MAXIMUM_... / build-and-push-image
- GitHub Check: plgd-device-test (cloud-server-release-discovery-resource-observable-access-in-RFOTM-rep-realloc,... / plgd-device-test-with-cfg
- GitHub Check: plgd-hub-test (dps-cloud-server-tsan, -DOC_TSAN_ENABLED=ON -DCMAKE_CXX_COMPILER=clang++ -DCMAKE_C... / plgd-hub-test-with-cfg
- GitHub Check: build-and-push-images (cloud-server, Release, docker/apps/Dockerfile.cloud-server) / build-and-push-image
- GitHub Check: build-and-push-dps-images (dps-cloud-server-debug, Debug, -DOC_DEBUG_ENABLED=ON -DOC_LOG_MAXIMUM_... / build-and-push-image
- GitHub Check: plgd-device-test (cloud-server-release-discovery-resource-observable-access-in-RFOTM-rep-realloc,... / plgd-device-test-with-cfg
- GitHub Check: plgd-hub-test (dps-cloud-server-tsan, -DOC_TSAN_ENABLED=ON -DCMAKE_CXX_COMPILER=clang++ -DCMAKE_C... / plgd-hub-test-with-cfg
- GitHub Check: build-and-push-images (cloud-server, Release, docker/apps/Dockerfile.cloud-server) / build-and-push-image
- GitHub Check: build-and-push-dps-images (dps-cloud-server-debug, Debug, -DOC_DEBUG_ENABLED=ON -DOC_LOG_MAXIMUM_... / build-and-push-image
- GitHub Check: plgd-device-test (cloud-server-release-discovery-resource-observable-access-in-RFOTM-rep-realloc,... / plgd-device-test-with-cfg
- GitHub Check: plgd-hub-test (dps-cloud-server-tsan, -DOC_TSAN_ENABLED=ON -DCMAKE_CXX_COMPILER=clang++ -DCMAKE_C... / plgd-hub-test-with-cfg
- GitHub Check: build-and-push-images (cloud-server, Release, docker/apps/Dockerfile.cloud-server) / build-and-push-image
- GitHub Check: build-and-push-dps-images (dps-cloud-server-debug, Debug, -DOC_DEBUG_ENABLED=ON -DOC_LOG_MAXIMUM_... / build-and-push-image
- GitHub Check: plgd-device-test (cloud-server-release-discovery-resource-observable-access-in-RFOTM-rep-realloc,... / plgd-device-test-with-cfg
- GitHub Check: plgd-hub-test (dps-cloud-server-tsan, -DOC_TSAN_ENABLED=ON -DCMAKE_CXX_COMPILER=clang++ -DCMAKE_C... / plgd-hub-test-with-cfg
- GitHub Check: build-and-push-images (cloud-server, Release, docker/apps/Dockerfile.cloud-server) / build-and-push-image
- GitHub Check: build-and-push-dps-images (dps-cloud-server-debug, Debug, -DOC_DEBUG_ENABLED=ON -DOC_LOG_MAXIMUM_... / build-and-push-image
- GitHub Check: plgd-device-test (cloud-server-release-discovery-resource-observable-access-in-RFOTM-rep-realloc,... / plgd-device-test-with-cfg
- GitHub Check: plgd-hub-test (dps-cloud-server-tsan, -DOC_TSAN_ENABLED=ON -DCMAKE_CXX_COMPILER=clang++ -DCMAKE_C... / plgd-hub-test-with-cfg
- GitHub Check: build-and-push-images (cloud-server, Release, docker/apps/Dockerfile.cloud-server) / build-and-push-image
- GitHub Check: build-and-push-dps-images (dps-cloud-server-debug, Debug, -DOC_DEBUG_ENABLED=ON -DOC_LOG_MAXIMUM_... / build-and-push-image
- GitHub Check: plgd-device-test (cloud-server-release-discovery-resource-observable-access-in-RFOTM-rep-realloc,... / plgd-device-test-with-cfg
- GitHub Check: plgd-hub-test (dps-cloud-server-tsan, -DOC_TSAN_ENABLED=ON -DCMAKE_CXX_COMPILER=clang++ -DCMAKE_C... / plgd-hub-test-with-cfg
- GitHub Check: build-and-push-images (cloud-server, Release, docker/apps/Dockerfile.cloud-server) / build-and-push-image
- GitHub Check: build-and-push-dps-images (dps-cloud-server-debug, Debug, -DOC_DEBUG_ENABLED=ON -DOC_LOG_MAXIMUM_... / build-and-push-image
- GitHub Check: plgd-device-test (cloud-server-release-discovery-resource-observable-access-in-RFOTM-rep-realloc,... / plgd-device-test-with-cfg
- GitHub Check: plgd-hub-test (dps-cloud-server-tsan, -DOC_TSAN_ENABLED=ON -DCMAKE_CXX_COMPILER=clang++ -DCMAKE_C... / plgd-hub-test-with-cfg
- GitHub Check: build-and-push-images (cloud-server, Release, docker/apps/Dockerfile.cloud-server) / build-and-push-image
- GitHub Check: build-and-push-dps-images (dps-cloud-server-debug, Debug, -DOC_DEBUG_ENABLED=ON -DOC_LOG_MAXIMUM_... / build-and-push-image
- GitHub Check: plgd-device-test (cloud-server-release-discovery-resource-observable-access-in-RFOTM-rep-realloc,... / plgd-device-test-with-cfg
- GitHub Check: plgd-hub-test (dps-cloud-server-tsan, -DOC_TSAN_ENABLED=ON -DCMAKE_CXX_COMPILER=clang++ -DCMAKE_C... / plgd-hub-test-with-cfg
- GitHub Check: build-and-push-images (cloud-server, Release, docker/apps/Dockerfile.cloud-server) / build-and-push-image
- GitHub Check: build-and-push-dps-images (dps-cloud-server-debug, Debug, -DOC_DEBUG_ENABLED=ON -DOC_LOG_MAXIMUM_... / build-and-push-image
- GitHub Check: plgd-device-test (cloud-server-release-discovery-resource-observable-access-in-RFOTM-rep-realloc,... / plgd-device-test-with-cfg
- GitHub Check: plgd-hub-test (dps-cloud-server-tsan, -DOC_TSAN_ENABLED=ON -DCMAKE_CXX_COMPILER=clang++ -DCMAKE_C... / plgd-hub-test-with-cfg
- GitHub Check: build-and-push-images (cloud-server, Release, docker/apps/Dockerfile.cloud-server) / build-and-push-image
- GitHub Check: build-and-push-dps-images (dps-cloud-server-debug, Debug, -DOC_DEBUG_ENABLED=ON -DOC_LOG_MAXIMUM_... / build-and-push-image
- GitHub Check: plgd-device-test (cloud-server-release-discovery-resource-observable-access-in-RFOTM-rep-realloc,... / plgd-device-test-with-cfg
- GitHub Check: plgd-hub-test (dps-cloud-server-tsan, -DOC_TSAN_ENABLED=ON -DCMAKE_CXX_COMPILER=clang++ -DCMAKE_C... / plgd-hub-test-with-cfg
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
CMakeLists.txt (2)
640-642: Prefertarget_link_optionsfor linker flags instead of mixing them into libraries
Linker flags like-lgcovor--coverageare currently appended viaPRIVATE_LINK_LIBS. Since CMake 3.13+, it’s clearer to usetarget_link_options(<target> PRIVATE ${COVERAGE_LINK_OPTIONS})(or a globaladd_link_options) to distinguish flags from actual libraries.
893-896: Redundant compile/link options on a static library target
target_compile_options(client-server-static PRIVATE ...)andtarget_link_libraries(client-server-static PRIVATE ...)have no effect for a static library (it doesn’t compile or link sources itself). You can remove these lines or switch them toINTERFACEif the intention is to propagate coverage flags to consumers.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
CMakeLists.txt(3 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (30)
- GitHub Check: plgd-device-tests (cloud-server-access-in-RFOTM-concurrent-requests-1, -DOC_RESOURCE_ACCESS_IN_RF... / plgd-device-test-with-cfg
- GitHub Check: plgd-hub-tests (cloud-server-discovery-resource-observable-access-in-RFOTM-rep-realloc-concurrent... / plgd-hub-test-with-cfg
- GitHub Check: cmake_linux (-DOC_IPV4_ENABLED=ON) / unit-test-with-cfg
- GitHub Check: plgd-device-tests (cloud-server-access-in-RFOTM-concurrent-requests-1, -DOC_RESOURCE_ACCESS_IN_RF... / plgd-device-test-with-cfg
- GitHub Check: plgd-hub-tests (cloud-server-discovery-resource-observable-access-in-RFOTM-rep-realloc-concurrent... / plgd-hub-test-with-cfg
- GitHub Check: cmake_linux (-DOC_IPV4_ENABLED=ON) / unit-test-with-cfg
- GitHub Check: plgd-device-tests (cloud-server-access-in-RFOTM-concurrent-requests-1, -DOC_RESOURCE_ACCESS_IN_RF... / plgd-device-test-with-cfg
- GitHub Check: plgd-hub-tests (cloud-server-discovery-resource-observable-access-in-RFOTM-rep-realloc-concurrent... / plgd-hub-test-with-cfg
- GitHub Check: cmake_linux (-DOC_IPV4_ENABLED=ON) / unit-test-with-cfg
- GitHub Check: plgd-device-tests (cloud-server-access-in-RFOTM-concurrent-requests-1, -DOC_RESOURCE_ACCESS_IN_RF... / plgd-device-test-with-cfg
- GitHub Check: plgd-hub-tests (cloud-server-discovery-resource-observable-access-in-RFOTM-rep-realloc-concurrent... / plgd-hub-test-with-cfg
- GitHub Check: cmake_linux (-DOC_IPV4_ENABLED=ON) / unit-test-with-cfg
- GitHub Check: plgd-device-tests (cloud-server-access-in-RFOTM-concurrent-requests-1, -DOC_RESOURCE_ACCESS_IN_RF... / plgd-device-test-with-cfg
- GitHub Check: plgd-hub-tests (cloud-server-discovery-resource-observable-access-in-RFOTM-rep-realloc-concurrent... / plgd-hub-test-with-cfg
- GitHub Check: cmake_linux (-DOC_IPV4_ENABLED=ON) / unit-test-with-cfg
- GitHub Check: plgd-device-tests (cloud-server-access-in-RFOTM-concurrent-requests-1, -DOC_RESOURCE_ACCESS_IN_RF... / plgd-device-test-with-cfg
- GitHub Check: plgd-hub-tests (cloud-server-discovery-resource-observable-access-in-RFOTM-rep-realloc-concurrent... / plgd-hub-test-with-cfg
- GitHub Check: cmake_linux (-DOC_IPV4_ENABLED=ON) / unit-test-with-cfg
- GitHub Check: plgd-device-tests (cloud-server-access-in-RFOTM-concurrent-requests-1, -DOC_RESOURCE_ACCESS_IN_RF... / plgd-device-test-with-cfg
- GitHub Check: plgd-hub-tests (cloud-server-discovery-resource-observable-access-in-RFOTM-rep-realloc-concurrent... / plgd-hub-test-with-cfg
- GitHub Check: cmake_linux (-DOC_IPV4_ENABLED=ON) / unit-test-with-cfg
- GitHub Check: plgd-device-tests (cloud-server-access-in-RFOTM-concurrent-requests-1, -DOC_RESOURCE_ACCESS_IN_RF... / plgd-device-test-with-cfg
- GitHub Check: plgd-hub-tests (cloud-server-discovery-resource-observable-access-in-RFOTM-rep-realloc-concurrent... / plgd-hub-test-with-cfg
- GitHub Check: cmake_linux (-DOC_IPV4_ENABLED=ON) / unit-test-with-cfg
- GitHub Check: plgd-device-tests (cloud-server-access-in-RFOTM-concurrent-requests-1, -DOC_RESOURCE_ACCESS_IN_RF... / plgd-device-test-with-cfg
- GitHub Check: plgd-hub-tests (cloud-server-discovery-resource-observable-access-in-RFOTM-rep-realloc-concurrent... / plgd-hub-test-with-cfg
- GitHub Check: cmake_linux (-DOC_IPV4_ENABLED=ON) / unit-test-with-cfg
- GitHub Check: plgd-device-tests (cloud-server-access-in-RFOTM-concurrent-requests-1, -DOC_RESOURCE_ACCESS_IN_RF... / plgd-device-test-with-cfg
- GitHub Check: plgd-hub-tests (cloud-server-discovery-resource-observable-access-in-RFOTM-rep-realloc-concurrent... / plgd-hub-test-with-cfg
- GitHub Check: cmake_linux (-DOC_IPV4_ENABLED=ON) / unit-test-with-cfg
Danielius1922
force-pushed
the
adam/feature/654-coverity-fixes-1
branch
from
41915e9 to
1941bee
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (2)
tools/collect-coverage.sh (2)
102-109: 🛠️ Refactor suggestionImprove Clang coverage tool detection and version parsing
The current logic:
- Uses
which llvm-cov-${CLANG_VERSION} 2>/dev/null 1>&2(confusing redirection ordering; prefer standardcommand -v ... >/dev/null 2>&1).- Extracts the major version via
$4(brittle across differentclang --versionformats).- Assigns
GCOVas a single string"llvm-cov-${CLANG_VERSION} gcov", which won’t split into two tokens when invoked.Apply these diffs to make detection robust and POSIX-compliant:
- clang --version - CLANG_VERSION=$(clang --version | awk 'NR==1 { split($4, v, "."); print v[1] }') - if ! which llvm-cov-${CLANG_VERSION} 2>/dev/null 1>&2; then + clang --version + # Extract first numeric token for robust version parsing + CLANG_VERSION=$(clang --version | awk 'NR==1 { + for (i=1; i<=NF; i++) if ($i ~ /^[0-9]+(\.[0-9]+)*/) { + split($i, v, "."); print v[1]; exit + } + }') + if ! command -v "llvm-cov-${CLANG_VERSION}" >/dev/null 2>&1; then echo "llvm-cov-${CLANG_VERSION} not installed" >&2 exit 1 fi - GCOV="llvm-cov-${CLANG_VERSION} gcov" + # Use array to preserve command + subcommand + GCOV=( "llvm-cov-${CLANG_VERSION}" "gcov" )Note: Update the later invocation to expand the array, e.g.
--gcov-executable "${GCOV[0]}"and incorporate"${GCOV[@]:1}"if needed.
111-119: 🛠️ Refactor suggestionImprove GCC coverage tool detection and version parsing
The GCC branch mirrors the Clang logic and has the same issues:
- Relies on field
$4for version extraction.- Uses
which … 2>/dev/null 1>&2instead ofcommand -v.Proposed diff:
- gcc --version - GCC_VERSION=$(gcc --version | awk 'NR==1 { split($4, v, "."); print v[1] }') - if ! which gcov-${GCC_VERSION} 2>/dev/null 1>&2; then + gcc --version + # Extract first numeric token for robust version parsing + GCC_VERSION=$(gcc --version | awk 'NR==1 { + for (i=1; i<=NF; i++) if ($i ~ /^[0-9]+(\.[0-9]+)*/) { + split($i, v, "."); print v[1]; exit + } + }') + if ! command -v "gcov-${GCC_VERSION}" >/dev/null 2>&1; then echo "gcov-${GCC_VERSION} not installed" >&2 exit 1 fi - GCOV="gcov-${GCC_VERSION}" + GCOV="gcov-${GCC_VERSION}"
🧹 Nitpick comments (1)
tools/collect-coverage.sh (1)
143-146: Optional: Consolidate GCOVR version checksThe script currently uses two separate
awkinvocations to detectgcovrv5.0+ and v6.0+. Consider refactoring into a small helper function to DRY up the logic and make future version thresholds trivial to add:add_gcovr_opt() { local min_version=$1 opt_key=$2 opt_val=$3 if awk "BEGIN {exit !(${GCOVR_VERSION} >= ${min_version})}"; then echo "gcovr v${min_version}+ detected" GCOVR_OPTS+=("${opt_key}" "${opt_val}") fi } # then simply call: add_gcovr_opt 5.0 "--exclude-lines-by-pattern" "${pattern}" add_gcovr_opt 6.0 "--merge-mode-functions" "separate"
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (13)
CMakeLists.txt(3 hunks)api/oc_client_api.c(2 hunks)api/unittest/collectiontest.cpp(14 hunks)api/unittest/discovery/discovery.cpp(8 hunks)api/unittest/discovery/discovery.h(1 hunks)api/unittest/discovery/discoveryobservetest.cpp(2 hunks)api/unittest/etagtest.cpp(4 hunks)api/unittest/resourcetest.cpp(6 hunks)messaging/coap/engine.c(1 hunks)messaging/coap/observe.c(2 hunks)messaging/coap/unittest/observenotificationstest.cpp(10 hunks)tests/gtest/Collection.h(1 hunks)tools/collect-coverage.sh(2 hunks)
✅ Files skipped from review due to trivial changes (4)
- api/unittest/discovery/discoveryobservetest.cpp
- messaging/coap/observe.c
- api/unittest/discovery/discovery.h
- api/unittest/collectiontest.cpp
🚧 Files skipped from review as they are similar to previous changes (3)
- api/unittest/resourcetest.cpp
- api/unittest/discovery/discovery.cpp
- CMakeLists.txt
🧰 Additional context used
🧬 Code Graph Analysis (1)
messaging/coap/engine.c (2)
api/oc_helpers.c (1)
oc_string_len_unsafe(221-226)api/oc_blockwise.c (1)
oc_blockwise_alloc_response_buffer(183-210)
⏰ Context from checks skipped due to timeout of 90000ms (20)
- GitHub Check: unit-tests (-DOC_CLOUD_ENABLED=ON -DOC_DYNAMIC_ALLOCATION_ENABLED=OFF -DOC_RESOURCE_ACCESS_IN_RFO... / unit-test-with-cfg
- GitHub Check: unit-tests (-DOC_DNS_LOOKUP_IPV6_ENABLED=ON -DOC_OSCORE_ENABLED=OFF -DOC_REPRESENTATION_REALLOC_E... / unit-test-with-cfg
- GitHub Check: unit-tests (-DOC_CLOUD_ENABLED=ON -DOC_DYNAMIC_ALLOCATION_ENABLED=OFF -DOC_RESOURCE_ACCESS_IN_RFO... / unit-test-with-cfg
- GitHub Check: unit-tests (-DOC_DNS_LOOKUP_IPV6_ENABLED=ON -DOC_OSCORE_ENABLED=OFF -DOC_REPRESENTATION_REALLOC_E... / unit-test-with-cfg
- GitHub Check: unit-tests (-DOC_CLOUD_ENABLED=ON -DOC_DYNAMIC_ALLOCATION_ENABLED=OFF -DOC_RESOURCE_ACCESS_IN_RFO... / unit-test-with-cfg
- GitHub Check: unit-tests (-DOC_DNS_LOOKUP_IPV6_ENABLED=ON -DOC_OSCORE_ENABLED=OFF -DOC_REPRESENTATION_REALLOC_E... / unit-test-with-cfg
- GitHub Check: unit-tests (-DOC_CLOUD_ENABLED=ON -DOC_DYNAMIC_ALLOCATION_ENABLED=OFF -DOC_RESOURCE_ACCESS_IN_RFO... / unit-test-with-cfg
- GitHub Check: unit-tests (-DOC_DNS_LOOKUP_IPV6_ENABLED=ON -DOC_OSCORE_ENABLED=OFF -DOC_REPRESENTATION_REALLOC_E... / unit-test-with-cfg
- GitHub Check: unit-tests (-DOC_CLOUD_ENABLED=ON -DOC_DYNAMIC_ALLOCATION_ENABLED=OFF -DOC_RESOURCE_ACCESS_IN_RFO... / unit-test-with-cfg
- GitHub Check: unit-tests (-DOC_DNS_LOOKUP_IPV6_ENABLED=ON -DOC_OSCORE_ENABLED=OFF -DOC_REPRESENTATION_REALLOC_E... / unit-test-with-cfg
- GitHub Check: unit-tests (-DOC_CLOUD_ENABLED=ON -DOC_DYNAMIC_ALLOCATION_ENABLED=OFF -DOC_RESOURCE_ACCESS_IN_RFO... / unit-test-with-cfg
- GitHub Check: unit-tests (-DOC_DNS_LOOKUP_IPV6_ENABLED=ON -DOC_OSCORE_ENABLED=OFF -DOC_REPRESENTATION_REALLOC_E... / unit-test-with-cfg
- GitHub Check: unit-tests (-DOC_CLOUD_ENABLED=ON -DOC_DYNAMIC_ALLOCATION_ENABLED=OFF -DOC_RESOURCE_ACCESS_IN_RFO... / unit-test-with-cfg
- GitHub Check: unit-tests (-DOC_DNS_LOOKUP_IPV6_ENABLED=ON -DOC_OSCORE_ENABLED=OFF -DOC_REPRESENTATION_REALLOC_E... / unit-test-with-cfg
- GitHub Check: unit-tests (-DOC_CLOUD_ENABLED=ON -DOC_DYNAMIC_ALLOCATION_ENABLED=OFF -DOC_RESOURCE_ACCESS_IN_RFO... / unit-test-with-cfg
- GitHub Check: unit-tests (-DOC_DNS_LOOKUP_IPV6_ENABLED=ON -DOC_OSCORE_ENABLED=OFF -DOC_REPRESENTATION_REALLOC_E... / unit-test-with-cfg
- GitHub Check: unit-tests (-DOC_CLOUD_ENABLED=ON -DOC_DYNAMIC_ALLOCATION_ENABLED=OFF -DOC_RESOURCE_ACCESS_IN_RFO... / unit-test-with-cfg
- GitHub Check: unit-tests (-DOC_DNS_LOOKUP_IPV6_ENABLED=ON -DOC_OSCORE_ENABLED=OFF -DOC_REPRESENTATION_REALLOC_E... / unit-test-with-cfg
- GitHub Check: unit-tests (-DOC_CLOUD_ENABLED=ON -DOC_DYNAMIC_ALLOCATION_ENABLED=OFF -DOC_RESOURCE_ACCESS_IN_RFO... / unit-test-with-cfg
- GitHub Check: unit-tests (-DOC_DNS_LOOKUP_IPV6_ENABLED=ON -DOC_OSCORE_ENABLED=OFF -DOC_REPRESENTATION_REALLOC_E... / unit-test-with-cfg
🔇 Additional comments (22)
api/oc_client_api.c (2)
239-242: Added safety check for URI format before processing.The code now correctly validates that the URI begins with a slash character before advancing past it, preventing potential issues with malformed URIs. This change improves robustness by making an implicit assumption explicit through the assertion.
447-447: Improved input validation for URIs.Adding an assertion to verify that URIs start with a '/' character is a good defensive programming practice. This ensures that all URIs passed to the request function follow the expected format.
messaging/coap/engine.c (1)
996-999: Added proper URI validation and handling.This change implements the same URI validation pattern seen in
oc_client_api.c, ensuring consistency across the codebase. The assertion that the URI begins with '/' followed by adjusting the pointer and length is a good security practice that makes the code more robust against malformed inputs.tests/gtest/Collection.h (2)
70-70: Function signature updated for safer string handlingThe parameter types for
NewCollectionhave been changed fromstd::string_viewtoconst std::string &, which aligns with other similar changes across the codebase to unify string handling.
73-73: Updated string access method for C function compatibilityChanged from
.data()to.c_str()to ensure null-terminated strings are passed tooc_new_collection, which is a C function that expects C-style strings. This is a safer approach that prevents potential issues with non-null-terminated strings.messaging/coap/unittest/observenotificationstest.cpp (10)
62-68: String type consistency improvementChanged from
constexpr std::string_viewtostatic const std::stringfor URI constants, which provides more consistent string handling and avoids potential issues when passing to C functions that expect null-terminated strings.
164-165: Updated resource definition to use std::stringModified to use string directly in
makeDynamicResourceToAddcall, consistent with the string type changes throughout the codebase.
186-187: Added explicit C-string conversionAdded
.c_str()calls when passing strings tooc_collection_add_supported_rtandoc_collection_add_mandatory_rt, ensuring proper null-terminated strings for these C functions.
252-253: Using c_str() for C API compatibilityUpdated to use
.c_str()when passing the URI tooc_ri_get_app_resource_by_uri, which expects a C-style string.
269-269: Updated to pass std::string to RegisterObserveChanged to pass the string directly to
message::tcp::RegisterObserve, aligning with changes to usestd::stringthroughout the codebase.
303-304: URI string handling consistencyUpdated string handling when calling
oc_get_collection_by_urito use.c_str()for the URI parameter, ensuring proper null-terminated strings.
320-321: String handling in TCP message registrationUpdated string handling in message registration to directly use the string constant, consistent with string type changes throughout the codebase.
363-364: Consistent string handling in observer registrationSimilar to previous changes, updated string handling in the second test case to use consistent string passing approach.
481-483: Updated resource type usageChanged to use string directly in resource type definition for collection resource, improving type consistency.
512-513: String handling consistency in dynamic resourceUpdated string parameters in
makeDynamicResourceToAddto use the string directly rather than string_view, consistent with other similar changes.api/unittest/etagtest.cpp (7)
80-84: Improved string type consistencyChanged from
constexpr std::string_viewtostatic const std::stringfor URI constants, providing more consistent string handling and avoiding potential issues when passing to C functions.
484-491: Enhanced string handling in dynamic resource creationUpdated string handling in the dynamic resource function calls to use string object references directly, improving type safety and consistency.
499-499: Local string type consistencyChanged the
powerSwitchRTvariable from string_view to string, maintaining consistency with other string handling changes throughout the codebase.
504-505: Added C-string conversion for C API callsAdded
.c_str()calls when passing strings tooc_collection_add_supported_rtandoc_collection_add_mandatory_rt, ensuring proper null-terminated strings for these C functions.
512-513: String handling consistency in resource creationUpdated string parameters in
makeDynamicResourceToAddto use string objects directly instead of string_view, maintaining type consistency.
1054-1055: String handling improvement in collection URI lookupUpdated to use
.c_str()when passing the URI tooc_get_collection_by_uri, ensuring proper null-terminated strings.
1063-1064: String handling consistency in link URI lookupUpdated to use
.c_str()when passing the URI tooc_get_link_by_uri, ensuring proper null-terminated strings when interfacing with C functions.
Danielius1922
force-pushed
the
adam/feature/654-coverity-fixes-1
branch
3 times, most recently
from
17b5a5c to
68ed56b
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
port/linux/ipadapter.c (1)
374-374: Keep space around bitwise AND operator for better readability.The removal of the space before the
&operator makes the code less readable. Most C style guides recommend spaces around binary operators.- if (*(uint32_t *)(RTA_DATA(attr))&IFA_F_TEMPORARY) { + if (*(uint32_t *)(RTA_DATA(attr)) & IFA_F_TEMPORARY) {
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
port/linux/ipadapter.c(3 hunks)util/oc_compiler.h(2 hunks)
✅ Files skipped from review due to trivial changes (1)
- util/oc_compiler.h
🔇 Additional comments (5)
port/linux/ipadapter.c (5)
536-555: Good refactoring: Extracted interface refresh logic into a reusable function.This extraction improves code modularity and maintainability. The atomic operations are properly used to ensure thread-safe flag updates.
557-573: Well-designed wrapper functions for cleaner code.These wrapper functions effectively encapsulate the compiler warning suppression, making the code more maintainable and following the DRY principle.
575-626: Well-structured function for handling interface addition events.The function properly:
- Validates input parameters
- Handles both IPv4 and IPv6 cases with appropriate conditional compilation
- Accumulates error status correctly
- Uses the wrapper functions effectively
628-640: Clean implementation for interface removal handling.Simple and focused function that properly handles the interface removal event with appropriate conditional compilation.
646-696: Excellent refactoring of the event processing function.The function is now more modular and easier to understand by delegating specific event handling to dedicated functions. This improves:
- Code readability
- Maintainability
- Testability
- Separation of concerns
Danielius1922
force-pushed
the
adam/feature/654-coverity-fixes-1
branch
from
7192f32 to
bd2b556
Compare
✅ Actions performedReview triggered.
|
ocf-conformance-test-tool
bot
removed
the
OCF Conformance Testing
Fix issue 448975: Resource leak in oc_endpoint.c Fix issue 420846: Overflowed integer argument in plgd_dps_dhcp.c Fix issue 487972: Copy into fixed sized buffer in cloud_server.c Fix issue 55728: Out-of-bouds access in oc_client_api.c Fix issue 58116: Out-of-bouds access in engine.c Fix issue 58444: Out-of-bouds access in observe.c Fix issue 72327: Out-of-bouds access in dps_security Fix issues 73594, 73889: Out-of-bouds write in storage.c Fix issue 55551: speculative execution data leak in linux/ipadapter.c Fix issue 525128: Unused value in observe.c Fix issue 525129: Unused value in oc_server_api.c
Danielius1922
force-pushed
the
adam/feature/654-coverity-fixes-1
branch
from
52759ba to
5c1421c
Compare
ocf-conformance-test-tool
bot
removed
the
OCF Conformance Testing
|
No description provided.