fix: remediate P0 safety and mechanical clang-tidy violations (#222)#225
Merged
fix: remediate P0 safety and mechanical clang-tidy violations (#222)#225
Conversation
Contributor
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughReplaces nested C++ namespaces with C++17 inline namespaces across the tree; changes E2E CRC API to return std::optional and updates callers/tests; widens TP offset/length types; replaces pool init booleans with atomics; moves user callbacks out of mutexes; tightens const-correctness and initializes temporaries. Changes
Sequence Diagram(s)(omitted) Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
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. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 14
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (6)
src/platform/threadx/memory.cpp (1)
69-76:⚠️ Potential issue | 🔴 CriticalRemove
constqualifier fromblockpointer—incompatible with ThreadX and placement new.Line 69 declares
blockasvoid const*, but:
tx_block_allocate(&message_pool, &block, TX_NO_WAIT)expects aVOID**(non-const) parameternew (block) Message()expects avoid*(non-const) pointerPassing a
void const**totx_block_allocateand aconst void*to placement new are type mismatches that will cause compilation errors.Fix
- void const* block = nullptr; + void* block = nullptr;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/platform/threadx/memory.cpp` around lines 69 - 76, The local pointer `block` is declared as `void const*` which is incompatible with ThreadX's tx_block_allocate (expects `VOID**`) and placement new (expects `void*`); change the declaration of `block` to a non-const pointer (e.g., `void* block = nullptr`) so you can call `tx_block_allocate(&message_pool, &block, TX_NO_WAIT)` and use `new (block) Message()` safely; ensure subsequent uses (including the lambda destructor in `MessagePtr`) also treat the pointer as non-const.src/rpc/rpc_client.cpp (1)
80-89:⚠️ Potential issue | 🟠 MajorDo not invoke user callbacks while holding
pending_calls_mutex_.This can deadlock if a callback re-enters RPC client APIs that also take the same mutex (downstream impact:
cancel_callandon_message_receivedfollow the same pattern). Move callback collection inside the lock and execute callbacks after unlocking.Proposed pattern for `shutdown()` (apply similarly in `cancel_call` and `on_message_received`)
- { - platform::ScopedLock const lock(pending_calls_mutex_); - for (auto& pair : pending_calls_) { - if (pair.second.callback) { - RpcResponse response(pair.second.service_id, pair.second.method_id, - client_id_, pair.second.session_id, RpcResult::INTERNAL_ERROR); - pair.second.callback(response); - } - } - pending_calls_.clear(); - } + std::vector<std::pair<RpcCallback, RpcResponse>> callbacks; + { + platform::ScopedLock const lock(pending_calls_mutex_); + callbacks.reserve(pending_calls_.size()); + for (auto const& pair : pending_calls_) { + if (pair.second.callback) { + callbacks.emplace_back( + pair.second.callback, + RpcResponse(pair.second.service_id, pair.second.method_id, + client_id_, pair.second.session_id, RpcResult::INTERNAL_ERROR)); + } + } + pending_calls_.clear(); + } + for (auto& item : callbacks) { + item.first(item.second); + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/rpc/rpc_client.cpp` around lines 80 - 89, Do not invoke user callbacks while holding pending_calls_mutex_: in shutdown() (and similarly in cancel_call and on_message_received) acquire pending_calls_mutex_, iterate pending_calls_ to collect the RpcResponse objects or std::function callbacks into a local vector, clear pending_calls_ while still holding the lock, then release the lock and finally iterate the local vector to invoke each callback (using RpcResponse constructed with service_id, method_id, client_id_, session_id, RpcResult::INTERNAL_ERROR as you do now). This moves callback execution out of the locked region and prevents deadlocks when callbacks re-enter RPC client APIs that also lock pending_calls_mutex_.src/rpc/rpc_server.cpp (1)
127-133:⚠️ Potential issue | 🟠 MajorRelease
methods_mutex_before sending unknown-method response.At Line 131,
send_error_responseruns whilemethods_mutex_is still held. Move the send after the lock scope to avoid blocking method registration paths on network I/O.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/rpc/rpc_server.cpp` around lines 127 - 133, The code holds platform::ScopedLock methods_mutex_ while calling send_error_response, which can block registration paths; change the logic in the request handling (around methods_mutex_, method_handlers_, and the call that checks message->get_method_id()) so the lock is released before performing network I/O: acquire the lock, check method_handlers_ for message->get_method_id() and record whether the method exists (or store the iterator/result) then let the ScopedLock go out of scope; only after the lock is released, call send_error_response(message, sender, ReturnCode::E_UNKNOWN_METHOD) when the method was not found. Ensure you do not access method_handlers_ after releasing methods_mutex_.src/sd/sd_client.cpp (1)
377-391:⚠️ Potential issue | 🔴 CriticalInvoke user callbacks after releasing internal mutexes.
These callbacks execute while
subscriptions_mutex_/pending_finds_mutex_are held. Re-entrant client code can deadlock against subscribe/find APIs.Also applies to: 418-422
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/sd/sd_client.cpp` around lines 377 - 391, The code currently invokes user callbacks while holding internal mutexes (subscriptions_mutex_ and pending_finds_mutex_), risking deadlocks; modify the logic in the handler that processes instance notifications so it captures the callback(s) and any data (e.g., ServiceInstance) while under lock, then releases the lock(s) and invokes available_callback and pending_finds_ callbacks outside the critical sections; specifically, when you find sub_it->second.available_callback or an entry in pending_finds_ with it->second.callback, copy the std::function and the ServiceInstance into local variables/containers under the respective mutex (subscriptions_mutex_ / pending_finds_mutex_), erase/advance the containers as needed, then call the copied callbacks after the locks are dropped (do the same for the other occurrence referenced in the review).src/sd/sd_server.cpp (1)
112-143:⚠️ Potential issue | 🟠 MajorAvoid network sends while
offered_services_mutex_is held.The lock scope includes
send_service_offer(Line 142). If send blocks, service-management APIs are blocked behind the mutex. Snapshot data under lock, then send outside it.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/sd/sd_server.cpp` around lines 112 - 143, The code currently holds offered_services_mutex_ while calling send_service_offer, which can block; reduce the lock scope by assembling and inserting the OfferedService under the lock but capturing a copy (not a reference) of the element to send, then release the mutex and call send_service_offer on that copy. Specifically, keep the find/insert/eviction logic and offered_services_.push_back(std::move(offered)) protected by offered_services_mutex_, then create a local OfferedService copy (e.g., copy the newly inserted element) while still holding the lock, let the lock go out of scope, and finally call send_service_offer with that local copy so send_service_offer is executed without holding offered_services_mutex_.src/events/event_subscriber.cpp (1)
261-303:⚠️ Potential issue | 🔴 CriticalDo not invoke external callbacks while holding internal mutexes.
At Line 261 and Line 290, locks are held while callback code is executed (Line 278, Line 299). Re-entrant callbacks can deadlock by calling APIs that need the same mutexes.
Suggested fix
- platform::ScopedLock const subs_lock(subscriptions_mutex_); + EventNotificationCallback notify_cb; + EventNotification notify_copy(service_id, 0, event_id); + { + platform::ScopedLock const subs_lock(subscriptions_mutex_); ... - if (sub_info.notification_callback) { - sub_info.notification_callback(notification); - } + notify_copy = notification; + notify_cb = sub_info.notification_callback; ... - platform::ScopedLock const field_lock(field_requests_mutex_); + } + if (notify_cb) { + notify_cb(notify_copy); + } + + EventNotificationCallback field_cb; + EventNotification field_notify(service_id, 0, event_id); + { + platform::ScopedLock const field_lock(field_requests_mutex_); ... - if (field_it->second) { - field_it->second(notification); - } + field_notify = notification; + field_cb = field_it->second; field_requests_.erase(field_it); - } + } + if (field_cb) { + field_cb(field_notify); + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/events/event_subscriber.cpp` around lines 261 - 303, You are calling external callbacks while holding internal mutexes (subscriptions_mutex_ and field_requests_mutex_) which can deadlock; fix by capturing necessary data and the callback under the lock and then invoking the callback after releasing the lock. For the subscriptions loop (subscriptions_, subscriptions_mutex_, sub_pair/sub_info, notification_callback) create and populate EventNotification and copy sub_info.notification_callback to a local std::function while still holding the lock, update sub_info.subscription.state and last_notification under the lock, then release the lock and call the copied callback. For the field response path (field_requests_, field_requests_mutex_, make_field_key, field_it) copy/construct the EventNotification and copy field_it->second into a local callback, erase the entry while holding the lock, release the lock, then invoke the copied callback. Ensure no external call paths run while either mutex is held.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@include/events/event_publisher.h`:
- Line 46: The inline defaulted destructor ~EventPublisher() must be moved
out-of-line to avoid deleting an incomplete std::unique_ptr<EventPublisherImpl>;
change the header to declare ~EventPublisher(); (no inline definition) and add a
definition in the .cpp: implement EventPublisher::~EventPublisher() = default;
inside the someip::events namespace in the .cpp after includes where
EventPublisherImpl is fully defined so the unique_ptr deleter sees a complete
type.
In `@include/events/event_subscriber.h`:
- Line 45: Declare the destructor for EventSubscriber in the header (change
~EventSubscriber() = default; to a non-inline declaration ~EventSubscriber();)
and add an out-of-line definition in the implementation translation unit: define
EventSubscriber::~EventSubscriber() = default; in the .cpp where
EventSubscriberImpl is fully defined and ensure that .cpp includes the header
that provides the complete definition of EventSubscriberImpl so the unique_ptr
deleter can be instantiated correctly.
In `@include/rpc/rpc_client.h`:
- Line 44: The header currently has an inline defaulted destructor (~RpcClient()
= default;) which requires the complete type of RpcClientImpl to destroy the
std::unique_ptr impl_; change the header to declare the destructor
(~RpcClient();) without definition and add an out-of-line definition in the .cpp
(implement RpcClient::~RpcClient() = default; in the translation unit that
includes the RpcClientImpl definition) so the unique_ptr can be destroyed with a
complete type; update includes in the .cpp so RpcClientImpl is visible there.
In `@include/rpc/rpc_server.h`:
- Line 58: Declare RpcServer's destructor as non-defaulted in the header (remove
the inline "= default" from ~RpcServer()) and provide an out-of-line definition
in the corresponding .cpp after the RpcServerImpl type is complete;
specifically, add a definition RpcServer::~RpcServer() = default; in the .cpp
(inside the someip::rpc namespace) after including the header that defines
RpcServerImpl so the std::unique_ptr<RpcServerImpl> member can be destroyed with
a complete type.
In `@include/sd/sd_client.h`:
- Line 45: The inline defaulted destructor ~SdClient() = default; should be
declared in the header but defined out-of-line to avoid undefined behavior with
the incomplete type std::unique_ptr<SdClientImpl>; change the header to declare
SdClient::~SdClient(); and add an out-of-line definition SdClient::~SdClient() =
default; in the source file after SdClientImpl is fully defined so the
unique_ptr can properly destroy its pointee.
In `@include/sd/sd_server.h`:
- Line 45: The inline defaulted destructor ~SdServer() needs to be moved
out-of-line to avoid requiring the definition of SdServerImpl in the header;
change the header to declare ~SdServer(); (keeping the forward declaration of
SdServerImpl and std::unique_ptr<SdServerImpl> intact) and implement
SdServer::~SdServer() = default; in the corresponding .cpp where SdServerImpl is
defined so destruction of the std::unique_ptr<SdServerImpl> happens with a
complete type.
In `@src/e2e/e2e_crc.cpp`:
- Around line 120-121: The cast to std::ptrdiff_t for iterator arithmetic (auto
first = data.begin() + static_cast<std::ptrdiff_t>(offset); and the subsequent
std::vector<uint8_t> slice(...) construction) needs explicit checks against
PTRDIFF_MAX to avoid overflow: before casting, validate that offset and length
are each <= PTRDIFF_MAX and that offset + length <= PTRDIFF_MAX and <=
data.size(), then perform the static_casts and iterator arithmetic; update the
checks around offset, length, and the creation of first/slice to reject inputs
that violate these limits.
In `@src/e2e/e2e_profiles/standard_profile.cpp`:
- Around line 211-215: The current validation treats header.counter ==
last_counter as valid which allows replayed frames; update the validation in
standard_profile.cpp so that only header.counter > last_counter sets
counter_valid to true (i.e., reject equality), and adjust any related comments
to reflect that equal counters are considered duplicates and invalid; locate and
modify the condition using header.counter, last_counter, and counter_valid
accordingly.
In `@src/e2e/e2e_protection.cpp`:
- Around line 79-82: The comment and code disagree: remove the stale comment and
the unnecessary const reference alias msg_copy and call
profile->validate(message, config) directly; update or delete the comment above
the Message binding so validate() invocation uses the original message and the
code no longer declares Message const& msg_copy.
In `@src/platform/freertos/memory.cpp`:
- Around line 70-71: The storage pointer used for placement new is declared as
const (void const* block) but placement new (new (void*) Message) requires a
non-const void* so the constructor can write into the buffer; change the
declaration of block to a non-const pointer (void* block) or explicitly cast
pool_buffer + i * sizeof(Message) to void* before calling placement new on
Message (references: pool_buffer, Message, msg and the placement new call).
In `@src/sd/sd_message.cpp`:
- Around line 319-321: The multicast option's length is being stored into the
wrong bytes (data[2]/data[3]) which overwrites the option type; update the
writes to store the uint16_t length into the SdOption length field bytes data[0]
and data[1] (i.e., set data[0] = (length >> 8) & 0xFF; data[1] = length & 0xFF;)
so the option type and subsequent payload remain intact and deserialization
succeeds.
In `@src/tp/tp_manager.cpp`:
- Line 188: The completion_callback_ is being invoked while holding
transfers_mutex_ (via platform::ScopedLock) in TpManager, which risks deadlock;
fix by capturing/moving a local copy of completion_callback_ and any required
callback arguments inside the critical section protected by transfers_mutex_
(using platform::ScopedLock), then release the lock and invoke the copied
completion_callback_ outside the lock (apply same change for the other
occurrences around lines 201-203), ensuring no user callback runs while
transfers_mutex_ is held.
In `@src/tp/tp_reassembler.cpp`:
- Around line 56-57: The computation offset_units = tp_header >> 4 then offset =
offset_units * 16 can overflow the uint16_t destination; guard the narrowing
before assigning to offset (or change offset's type). Update the code around
tp_header, offset_units and offset so you compute the byte offset in a wider
type (e.g., uint32_t), check or clamp the result against UINT16_MAX (or validate
it against expected max segment size), and only assign a safe value to the
uint16_t offset to prevent wraparound and incorrect reassembly.
In `@src/transport/tcp_transport.cpp`:
- Around line 242-243: The sockaddr_in variable addr is already zero-initialized
via brace initialization (sockaddr_in addr = {}), so remove the redundant
memset(&addr, 0, sizeof(addr)); calls (found alongside the addr declaration and
the similar instance at the later occurrence) to avoid duplicate zeroing; just
keep the brace-initialized declarations (e.g., sockaddr_in addr = {}) in the
functions where these appear.
---
Outside diff comments:
In `@src/events/event_subscriber.cpp`:
- Around line 261-303: You are calling external callbacks while holding internal
mutexes (subscriptions_mutex_ and field_requests_mutex_) which can deadlock; fix
by capturing necessary data and the callback under the lock and then invoking
the callback after releasing the lock. For the subscriptions loop
(subscriptions_, subscriptions_mutex_, sub_pair/sub_info, notification_callback)
create and populate EventNotification and copy sub_info.notification_callback to
a local std::function while still holding the lock, update
sub_info.subscription.state and last_notification under the lock, then release
the lock and call the copied callback. For the field response path
(field_requests_, field_requests_mutex_, make_field_key, field_it)
copy/construct the EventNotification and copy field_it->second into a local
callback, erase the entry while holding the lock, release the lock, then invoke
the copied callback. Ensure no external call paths run while either mutex is
held.
In `@src/platform/threadx/memory.cpp`:
- Around line 69-76: The local pointer `block` is declared as `void const*`
which is incompatible with ThreadX's tx_block_allocate (expects `VOID**`) and
placement new (expects `void*`); change the declaration of `block` to a
non-const pointer (e.g., `void* block = nullptr`) so you can call
`tx_block_allocate(&message_pool, &block, TX_NO_WAIT)` and use `new (block)
Message()` safely; ensure subsequent uses (including the lambda destructor in
`MessagePtr`) also treat the pointer as non-const.
In `@src/rpc/rpc_client.cpp`:
- Around line 80-89: Do not invoke user callbacks while holding
pending_calls_mutex_: in shutdown() (and similarly in cancel_call and
on_message_received) acquire pending_calls_mutex_, iterate pending_calls_ to
collect the RpcResponse objects or std::function callbacks into a local vector,
clear pending_calls_ while still holding the lock, then release the lock and
finally iterate the local vector to invoke each callback (using RpcResponse
constructed with service_id, method_id, client_id_, session_id,
RpcResult::INTERNAL_ERROR as you do now). This moves callback execution out of
the locked region and prevents deadlocks when callbacks re-enter RPC client APIs
that also lock pending_calls_mutex_.
In `@src/rpc/rpc_server.cpp`:
- Around line 127-133: The code holds platform::ScopedLock methods_mutex_ while
calling send_error_response, which can block registration paths; change the
logic in the request handling (around methods_mutex_, method_handlers_, and the
call that checks message->get_method_id()) so the lock is released before
performing network I/O: acquire the lock, check method_handlers_ for
message->get_method_id() and record whether the method exists (or store the
iterator/result) then let the ScopedLock go out of scope; only after the lock is
released, call send_error_response(message, sender,
ReturnCode::E_UNKNOWN_METHOD) when the method was not found. Ensure you do not
access method_handlers_ after releasing methods_mutex_.
In `@src/sd/sd_client.cpp`:
- Around line 377-391: The code currently invokes user callbacks while holding
internal mutexes (subscriptions_mutex_ and pending_finds_mutex_), risking
deadlocks; modify the logic in the handler that processes instance notifications
so it captures the callback(s) and any data (e.g., ServiceInstance) while under
lock, then releases the lock(s) and invokes available_callback and
pending_finds_ callbacks outside the critical sections; specifically, when you
find sub_it->second.available_callback or an entry in pending_finds_ with
it->second.callback, copy the std::function and the ServiceInstance into local
variables/containers under the respective mutex (subscriptions_mutex_ /
pending_finds_mutex_), erase/advance the containers as needed, then call the
copied callbacks after the locks are dropped (do the same for the other
occurrence referenced in the review).
In `@src/sd/sd_server.cpp`:
- Around line 112-143: The code currently holds offered_services_mutex_ while
calling send_service_offer, which can block; reduce the lock scope by assembling
and inserting the OfferedService under the lock but capturing a copy (not a
reference) of the element to send, then release the mutex and call
send_service_offer on that copy. Specifically, keep the find/insert/eviction
logic and offered_services_.push_back(std::move(offered)) protected by
offered_services_mutex_, then create a local OfferedService copy (e.g., copy the
newly inserted element) while still holding the lock, let the lock go out of
scope, and finally call send_service_offer with that local copy so
send_service_offer is executed without holding offered_services_mutex_.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: adec4986-e544-46d5-923c-3e4aadc46db7
📒 Files selected for processing (54)
.clang-tidyinclude/e2e/e2e_config.hinclude/e2e/e2e_crc.hinclude/e2e/e2e_header.hinclude/e2e/e2e_profile.hinclude/e2e/e2e_profile_registry.hinclude/e2e/e2e_protection.hinclude/events/event_publisher.hinclude/events/event_subscriber.hinclude/events/event_types.hinclude/platform/host/host_condition_variable.hinclude/platform/posix/memory_impl.hinclude/platform/posix/net_impl.hinclude/platform/posix/thread_impl.hinclude/platform/thread.hinclude/rpc/rpc_client.hinclude/rpc/rpc_server.hinclude/rpc/rpc_types.hinclude/sd/sd_client.hinclude/sd/sd_message.hinclude/sd/sd_server.hinclude/sd/sd_types.hinclude/serialization/serializer.hinclude/tp/tp_manager.hinclude/tp/tp_reassembler.hinclude/tp/tp_segmenter.hinclude/tp/tp_types.hinclude/transport/endpoint.hinclude/transport/tcp_transport.hinclude/transport/transport.hinclude/transport/udp_transport.hsrc/core/session_manager.cppsrc/e2e/e2e_crc.cppsrc/e2e/e2e_header.cppsrc/e2e/e2e_profile_registry.cppsrc/e2e/e2e_profiles/standard_profile.cppsrc/e2e/e2e_protection.cppsrc/events/event_publisher.cppsrc/events/event_subscriber.cppsrc/platform/freertos/memory.cppsrc/platform/threadx/memory.cppsrc/rpc/rpc_client.cppsrc/rpc/rpc_server.cppsrc/sd/sd_client.cppsrc/sd/sd_message.cppsrc/sd/sd_server.cppsrc/serialization/serializer.cppsrc/someip/message.cppsrc/tp/tp_manager.cppsrc/tp/tp_reassembler.cppsrc/tp/tp_segmenter.cppsrc/transport/endpoint.cppsrc/transport/tcp_transport.cppsrc/transport/udp_transport.cpp
vtz
force-pushed
the
feat/clang-tidy-remediation
branch
2 times, most recently
from
c4b2e0e to
383a399
Compare
Contributor
There was a problem hiding this comment.
Actionable comments posted: 5
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/rpc/rpc_client.cpp (1)
80-86:⚠️ Potential issue | 🔴 CriticalDo not invoke callbacks while holding
pending_calls_mutex_.Current paths call user callbacks under the lock, which can deadlock if callbacks call back into client APIs (
cancel_call, new calls, shutdown).🔧 Proposed pattern (unlock before callback)
- { - platform::ScopedLock const lock(pending_calls_mutex_); - for (auto& pair : pending_calls_) { - if (pair.second.callback) { - RpcResponse response(...); - pair.second.callback(response); - } - } - pending_calls_.clear(); - } + std::vector<std::pair<RpcCallback, RpcResponse>> callbacks; + { + platform::ScopedLock const lock(pending_calls_mutex_); + for (auto& pair : pending_calls_) { + if (pair.second.callback) { + callbacks.emplace_back( + pair.second.callback, + RpcResponse(pair.second.service_id, pair.second.method_id, + client_id_, pair.second.session_id, RpcResult::INTERNAL_ERROR)); + } + } + pending_calls_.clear(); + } + for (auto& cb : callbacks) { + cb.first(cb.second); + }Apply the same unlock-before-callback pattern to
cancel_call()andon_message_received().Also applies to: 182-193, 225-242
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/rpc/rpc_client.cpp` around lines 80 - 86, Callbacks are being invoked while holding pending_calls_mutex_ (see the loop that constructs RpcResponse and calls pair.second.callback), which can deadlock; change these sites to capture the callback (and any response object like RpcResponse constructed from pair.second.service_id/method_id/client_id_/session_id/RpcResult::INTERNAL_ERROR) and erase/update the pending_calls_ entry as needed while holding the lock, then release the lock and invoke the captured callback outside the critical section. Apply the same unlock-before-callback pattern to cancel_call() and on_message_received() where pending_calls_ entries are completed or cancelled so that mutex is not held during user callback execution.
♻️ Duplicate comments (8)
src/e2e/e2e_protection.cpp (1)
79-82:⚠️ Potential issue | 🟡 MinorStale
validate()comment and redundant alias still present.At Line 79-82, the comment says a mutable copy is required, but the code uses a const reference alias. Drop the stale wording and call
profile->validate(message, config)directly.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/e2e/e2e_protection.cpp` around lines 79 - 82, The code contains a stale comment and a redundant const reference alias that suggests a mutable copy is needed but actually creates a const alias; remove the comment and the alias and call profile->validate(message, config) directly. Locate the block using Message const& msg_copy = message and the surrounding comment, delete both, and replace the redundant alias usage so the call uses the original message and config with profile->validate(message, config). Ensure no other code depends on msg_copy before removing it.src/e2e/e2e_crc.cpp (1)
120-121:⚠️ Potential issue | 🟡 Minor
size_t→std::ptrdiff_tcast still lacks overflow guard (unresolved).Line 120/121 cast offsets directly to
std::ptrdiff_twithout checking againstptrdiff_tmax, so very large inputs can still overflow on narrowerptrdiff_ttargets.Suggested fix
+#include <limits> ... uint32_t calculate_crc(const std::vector<uint8_t>& data, size_t offset, size_t length, uint8_t crc_type) { if (offset > data.size() || length > data.size() || offset > data.size() - length) { return 0; } + const size_t max_diff = static_cast<size_t>(std::numeric_limits<std::ptrdiff_t>::max()); + if (offset > max_diff || length > max_diff || offset > max_diff - length) { + return 0; + } auto first = data.begin() + static_cast<std::ptrdiff_t>(offset); std::vector<uint8_t> slice(first, first + static_cast<std::ptrdiff_t>(length));#!/bin/bash # Verify whether ptrdiff_t-bound checks exist near calculate_crc iterator casts sed -n '115,126p' src/e2e/e2e_crc.cpp rg -n 'PTRDIFF_MAX|numeric_limits<\s*std::ptrdiff_t\s*>::max' src/e2e/e2e_crc.cpp🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/e2e/e2e_crc.cpp` around lines 120 - 121, The cast of offset and length into std::ptrdiff_t when building the iterator slice (variables first and slice) can overflow on platforms where ptrdiff_t is narrower; before casting, validate that offset and length are <= static_cast<size_t>(std::numeric_limits<std::ptrdiff_t>::max()) and that offset + length does not exceed data.size() (and check for offset + length overflow using size_t arithmetic), then perform the safe cast; if the checks fail, return/throw an appropriate error instead of performing the cast in the code that constructs first and slice.src/tp/tp_manager.cpp (1)
188-203:⚠️ Potential issue | 🔴 CriticalCallback is still invoked under
transfers_mutex_(unresolved).Line 188 holds the mutex, and Lines 201-203 call
completion_callback_inside that critical section, preserving the deadlock/re-entrancy risk flagged earlier.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/tp/tp_manager.cpp` around lines 188 - 203, Held transfers_mutex_ while invoking completion_callback_ creates re-entrancy/deadlock risk: modify the loop in tp_manager.cpp so you update transfer.state (TpTransfer), increment statistics_.timeouts and record the transfer.transfer_id and TpResult::TIMEOUT into a local list (vector) while holding transfers_mutex_, then release transfers_mutex_ before actually calling completion_callback_ for each recorded transfer_id; ensure you still remove/leave active_transfers_ as intended inside the locked section but perform completion_callback_ invocations only after the lock is released.src/sd/sd_message.cpp (1)
320-321:⚠️ Potential issue | 🔴 CriticalWrite multicast option length into header bytes 0/1, not 2/3.
Lines 320-321 currently overwrite the option type/reserved fields. The length field belongs in bytes 0-1.
Proposed fix
- data[2] = (length >> 8) & 0xFF; - data[3] = length & 0xFF; + data[0] = (length >> 8) & 0xFF; + data[1] = length & 0xFF;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/sd/sd_message.cpp` around lines 320 - 321, The multicast option length is being written to data[2]/data[3], overwriting the option type/reserved bytes; change the writes so the length is stored in header bytes data[0] and data[1] instead (keep using the existing length variable and data buffer), preserving data[2]/data[3] for option type/reserved—update the code around where data and length are set in sd_message.cpp to use data[0] = (length >> 8) & 0xFF and data[1] = length & 0xFF instead of data[2]/data[3].src/platform/freertos/memory.cpp (1)
70-71:⚠️ Potential issue | 🔴 CriticalPlacement-new target must be non-const storage.
Line 70 uses
void const*for placement-new storage, but Line 71 constructs into that buffer. Placement new requires writablevoid*storage.Proposed fix
- void const* block = pool_buffer + i * sizeof(Message); + void* block = static_cast<void*>(pool_buffer + i * sizeof(Message)); auto* msg = new (block) Message();#!/bin/bash set -euo pipefail # Verify placement-new call site and storage pointer type rg -n -C2 'void\s+const\*\s+block|new\s*\(\s*block\s*\)\s*Message\s*\(' src/platform/freertos/memory.cpp # Verify there is no custom placement-new overload accepting const void* rg -n -C2 'operator\s+new\s*\([^)]*const\s+void\s*\*' include src🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/platform/freertos/memory.cpp` around lines 70 - 71, The placement-new is constructing a Message into storage pointed to by a const pointer; change the storage pointer to writable storage so placement-new receives a void* (not void const*). Locate the allocation logic using pool_buffer and the local variable named block in the function that constructs Message with new (block) Message(), remove the const qualifier (or otherwise ensure pool_buffer is non-const) so block is void* before calling placement-new, and verify there are no custom placement-new overloads accepting const void* that would mask the error.src/tp/tp_reassembler.cpp (1)
56-57:⚠️ Potential issue | 🟠 MajorGuard narrowing before assigning TP offset to
uint16_t.Line 57 can truncate when
offset_units * 16exceeds0xFFFF, causing incorrect reassembly offsets.Proposed fix
uint32_t const offset_units = tp_header >> 4; - offset = offset_units * 16; // Convert back to bytes + uint32_t const offset_bytes = offset_units * 16U; // Convert back to bytes + if (offset_bytes > 0xFFFFU) { + return false; + } + offset = static_cast<uint16_t>(offset_bytes);#!/bin/bash set -euo pipefail # Verify current conversion path and absence of bounds check rg -n -C3 'offset_units|offset\s*=' src/tp/tp_reassembler.cpp🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/tp/tp_reassembler.cpp` around lines 56 - 57, The assignment computing offset from tp_header currently does "offset = offset_units * 16" which can overflow/narrow when offset (uint16_t) cannot hold offset_units*16; in the function handling TP reassembly (look for tp_header, offset_units, and offset in tp_reassembler.cpp) compute the product in a wider integer (e.g., uint32_t), check against UINT16_MAX (or the expected max reassembly offset) and handle out-of-range values explicitly (return an error, drop the packet, or clamp as your policy) before casting to uint16_t so no silent truncation occurs.src/transport/tcp_transport.cpp (1)
242-243: 🧹 Nitpick | 🔵 TrivialRemove redundant
memsetafter brace initialization.
addris already zero-initialized viasockaddr_in addr = {};, so the extramemsetis unnecessary in both sites.♻️ Proposed cleanup
- sockaddr_in addr = {}; - memset(&addr, 0, sizeof(addr)); + sockaddr_in addr{}; @@ - sockaddr_in addr = {}; - memset(&addr, 0, sizeof(addr)); + sockaddr_in addr{};Also applies to: 296-297
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/transport/tcp_transport.cpp` around lines 242 - 243, The sockaddr_in variable is already zero-initialized with "sockaddr_in addr = {}" so remove the redundant "memset(&addr, 0, sizeof(addr))" calls; update the code sites where "sockaddr_in addr = {}" is immediately followed by "memset(&addr, ...)" (e.g., the two occurrences around the TCP transport initialization) by deleting the memset lines and keeping the brace initialization only.src/e2e/e2e_profiles/standard_profile.cpp (1)
211-215:⚠️ Potential issue | 🔴 CriticalReject duplicate counters during validation.
Line 211 now accepts
header.counter == last_counter, which allows duplicate/replayed frames to pass the counter check.🛡️ Proposed fix
- } else if (header.counter >= last_counter) { - // Equal: same message re-validated (e.g. in tests). - // Greater: new message with higher counter. - // Both cases are valid. - counter_valid = true; + } else if (header.counter > last_counter) { + // Monotonic increase. + counter_valid = true; + } else if (header.counter == last_counter) { + // Duplicate frame/counter. + return Result::INVALID_ARGUMENT; } else {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/e2e/e2e_profiles/standard_profile.cpp` around lines 211 - 215, The current validation accepts header.counter == last_counter which allows duplicate/replayed frames; change the check that sets counter_valid so it only becomes true when header.counter > last_counter (strictly greater) and treat equal as invalid (set counter_valid = false and handle/log as a duplicate/replay). Update the surrounding comment to reflect that equal counters are now rejected; the change should be made where header.counter, last_counter, and counter_valid are used.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@include/serialization/serializer.h`:
- Around line 91-92: The default DeserializationResult currently sets error_ to
Result::SUCCESS while value_ is empty, causing is_success() to report success
but get_value()/move_value() to throw; fix this by updating the success logic so
that is_success() returns true only when error_ == Result::SUCCESS AND
value_.has_value() (or equivalently check value_ non-empty) or, alternatively,
initialize error_ to a non-success sentinel (e.g., Result::NO_VALUE) instead of
Result::SUCCESS; update references to value_, error_, is_success(), get_value(),
and move_value() accordingly so default-constructed objects never report success
without a contained value.
In `@src/platform/freertos/memory.cpp`:
- Around line 40-42: The read of pool_initialized is unsynchronized and can race
with the write done under the pool init mutex; fix by making the flag access
consistent: either turn pool_initialized into a std::atomic<bool> and use an
atomic load/store (or proper memory_order) or perform the read under the same
mutex used when writing it (i.e., implement double-checked locking: atomically
read, if false then lock the pool init mutex, re-check pool_initialized, perform
init, and set the flag while holding the mutex). Ensure all reads/writes of
pool_initialized use the chosen synchronized approach so no unsynchronized
access remains.
In `@src/platform/threadx/memory.cpp`:
- Around line 40-42: The fast-path check reads pool_initialized without
synchronization which races with the write guarded by the mutex later (write at
the init path around the code that sets pool_initialized). Fix by making
pool_initialized atomic<bool> (or by loading it with acquire semantics) and use
a second checked lock pattern: load with acquire, if false take the existing
mutex and re-check pool_initialized before initializing and storing true with
release; alternatively always read/set under the same mutex. Update uses of
pool_initialized to the chosen synchronized access so the unsynchronized read is
removed.
- Around line 69-70: The pointer `block` is declared as `void const*` but
`tx_block_allocate(&message_pool, &block, TX_NO_WAIT)` and the subsequent
placement-new need a mutable pointer; change the declaration of `block` to a
non-const pointer (e.g., `void* block` or `VOID* block`) so the call to
`tx_block_allocate` receives a `VOID**`-compatible address and the placement-new
at line 75 can write into the allocated memory.
In `@src/sd/sd_client.cpp`:
- Around line 377-381: The code currently invokes user callbacks (e.g.,
service_subscriptions_.second.available_callback and unavailable_callback, and
pending-find callbacks) while holding internal mutexes (subscriptions_mutex_,
pending_finds_mutex_), which can deadlock; fix by copying the callback (and any
small data needed, e.g., instance) out while under the lock, then release the
ScopedLock before calling the copied callback so the call happens without
holding subscriptions_mutex_ or pending_finds_mutex_; apply the same pattern to
all sites that call available_callback, unavailable_callback, and pending-find
callbacks so only a minimal check/assignment occurs under lock and the actual
external invocation happens after unlocking, taking care to keep a local copy to
ensure callback lifetime and to re-check existence under lock if necessary.
---
Outside diff comments:
In `@src/rpc/rpc_client.cpp`:
- Around line 80-86: Callbacks are being invoked while holding
pending_calls_mutex_ (see the loop that constructs RpcResponse and calls
pair.second.callback), which can deadlock; change these sites to capture the
callback (and any response object like RpcResponse constructed from
pair.second.service_id/method_id/client_id_/session_id/RpcResult::INTERNAL_ERROR)
and erase/update the pending_calls_ entry as needed while holding the lock, then
release the lock and invoke the captured callback outside the critical section.
Apply the same unlock-before-callback pattern to cancel_call() and
on_message_received() where pending_calls_ entries are completed or cancelled so
that mutex is not held during user callback execution.
---
Duplicate comments:
In `@src/e2e/e2e_crc.cpp`:
- Around line 120-121: The cast of offset and length into std::ptrdiff_t when
building the iterator slice (variables first and slice) can overflow on
platforms where ptrdiff_t is narrower; before casting, validate that offset and
length are <= static_cast<size_t>(std::numeric_limits<std::ptrdiff_t>::max())
and that offset + length does not exceed data.size() (and check for offset +
length overflow using size_t arithmetic), then perform the safe cast; if the
checks fail, return/throw an appropriate error instead of performing the cast in
the code that constructs first and slice.
In `@src/e2e/e2e_profiles/standard_profile.cpp`:
- Around line 211-215: The current validation accepts header.counter ==
last_counter which allows duplicate/replayed frames; change the check that sets
counter_valid so it only becomes true when header.counter > last_counter
(strictly greater) and treat equal as invalid (set counter_valid = false and
handle/log as a duplicate/replay). Update the surrounding comment to reflect
that equal counters are now rejected; the change should be made where
header.counter, last_counter, and counter_valid are used.
In `@src/e2e/e2e_protection.cpp`:
- Around line 79-82: The code contains a stale comment and a redundant const
reference alias that suggests a mutable copy is needed but actually creates a
const alias; remove the comment and the alias and call
profile->validate(message, config) directly. Locate the block using Message
const& msg_copy = message and the surrounding comment, delete both, and replace
the redundant alias usage so the call uses the original message and config with
profile->validate(message, config). Ensure no other code depends on msg_copy
before removing it.
In `@src/platform/freertos/memory.cpp`:
- Around line 70-71: The placement-new is constructing a Message into storage
pointed to by a const pointer; change the storage pointer to writable storage so
placement-new receives a void* (not void const*). Locate the allocation logic
using pool_buffer and the local variable named block in the function that
constructs Message with new (block) Message(), remove the const qualifier (or
otherwise ensure pool_buffer is non-const) so block is void* before calling
placement-new, and verify there are no custom placement-new overloads accepting
const void* that would mask the error.
In `@src/sd/sd_message.cpp`:
- Around line 320-321: The multicast option length is being written to
data[2]/data[3], overwriting the option type/reserved bytes; change the writes
so the length is stored in header bytes data[0] and data[1] instead (keep using
the existing length variable and data buffer), preserving data[2]/data[3] for
option type/reserved—update the code around where data and length are set in
sd_message.cpp to use data[0] = (length >> 8) & 0xFF and data[1] = length & 0xFF
instead of data[2]/data[3].
In `@src/tp/tp_manager.cpp`:
- Around line 188-203: Held transfers_mutex_ while invoking completion_callback_
creates re-entrancy/deadlock risk: modify the loop in tp_manager.cpp so you
update transfer.state (TpTransfer), increment statistics_.timeouts and record
the transfer.transfer_id and TpResult::TIMEOUT into a local list (vector) while
holding transfers_mutex_, then release transfers_mutex_ before actually calling
completion_callback_ for each recorded transfer_id; ensure you still
remove/leave active_transfers_ as intended inside the locked section but perform
completion_callback_ invocations only after the lock is released.
In `@src/tp/tp_reassembler.cpp`:
- Around line 56-57: The assignment computing offset from tp_header currently
does "offset = offset_units * 16" which can overflow/narrow when offset
(uint16_t) cannot hold offset_units*16; in the function handling TP reassembly
(look for tp_header, offset_units, and offset in tp_reassembler.cpp) compute the
product in a wider integer (e.g., uint32_t), check against UINT16_MAX (or the
expected max reassembly offset) and handle out-of-range values explicitly
(return an error, drop the packet, or clamp as your policy) before casting to
uint16_t so no silent truncation occurs.
In `@src/transport/tcp_transport.cpp`:
- Around line 242-243: The sockaddr_in variable is already zero-initialized with
"sockaddr_in addr = {}" so remove the redundant "memset(&addr, 0, sizeof(addr))"
calls; update the code sites where "sockaddr_in addr = {}" is immediately
followed by "memset(&addr, ...)" (e.g., the two occurrences around the TCP
transport initialization) by deleting the memset lines and keeping the brace
initialization only.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 2dca58ea-16bc-4f20-bdd3-dbc378aa9dc8
📒 Files selected for processing (54)
.clang-tidyinclude/e2e/e2e_config.hinclude/e2e/e2e_crc.hinclude/e2e/e2e_header.hinclude/e2e/e2e_profile.hinclude/e2e/e2e_profile_registry.hinclude/e2e/e2e_protection.hinclude/events/event_publisher.hinclude/events/event_subscriber.hinclude/events/event_types.hinclude/platform/host/host_condition_variable.hinclude/platform/posix/memory_impl.hinclude/platform/posix/net_impl.hinclude/platform/posix/thread_impl.hinclude/platform/thread.hinclude/rpc/rpc_client.hinclude/rpc/rpc_server.hinclude/rpc/rpc_types.hinclude/sd/sd_client.hinclude/sd/sd_message.hinclude/sd/sd_server.hinclude/sd/sd_types.hinclude/serialization/serializer.hinclude/tp/tp_manager.hinclude/tp/tp_reassembler.hinclude/tp/tp_segmenter.hinclude/tp/tp_types.hinclude/transport/endpoint.hinclude/transport/tcp_transport.hinclude/transport/transport.hinclude/transport/udp_transport.hsrc/core/session_manager.cppsrc/e2e/e2e_crc.cppsrc/e2e/e2e_header.cppsrc/e2e/e2e_profile_registry.cppsrc/e2e/e2e_profiles/standard_profile.cppsrc/e2e/e2e_protection.cppsrc/events/event_publisher.cppsrc/events/event_subscriber.cppsrc/platform/freertos/memory.cppsrc/platform/threadx/memory.cppsrc/rpc/rpc_client.cppsrc/rpc/rpc_server.cppsrc/sd/sd_client.cppsrc/sd/sd_message.cppsrc/sd/sd_server.cppsrc/serialization/serializer.cppsrc/someip/message.cppsrc/tp/tp_manager.cppsrc/tp/tp_reassembler.cppsrc/tp/tp_segmenter.cppsrc/transport/endpoint.cppsrc/transport/tcp_transport.cppsrc/transport/udp_transport.cpp
First batch of clang-tidy remediation per issue #222 plan. Config fix: - Fix .clang-tidy: snake_case -> lower_case (invalid option value, silently broke readability-identifier-naming checks) P0 — Safety (bugprone-*, init-variables, member-init): - bugprone-empty-catch: justify NOLINT for intentional thread creation catch in thread_impl.h - bugprone-exception-escape: wrap RpcClientImpl destructor in noexcept try-catch - bugprone-narrowing-conversions: cast size_t to ptrdiff_t for iterator arithmetic across 6 files - bugprone-branch-clone: merge identical if/else branches in standard_profile.cpp counter validation - cppcoreguidelines-init-variables: zero-initialize 19 uninitialized locals (memcpy targets, loop accumulators, output params) - cppcoreguidelines-pro-type-member-init: zero-init 18 struct records (sockaddr_in, ip_mreq, timeval) and add default member initializers P2 — Maintainability (mechanical, auto-fixed): - misc-const-correctness: add const to ~144 non-mutated locals - modernize-concat-nested-namespaces: convert all nested namespace declarations to C++17 syntax across 50+ files - readability-braces-around-statements: add braces to bare if/else - modernize-use-override, readability-static-accessed-through-instance, performance-avoid-endl, cppcoreguidelines-prefer-member-initializer, and other mechanical fixes Quality gate: PASSED (1505 violations, threshold 1783). Made-with: Cursor
vtz
force-pushed
the
feat/clang-tidy-remediation
branch
from
383a399 to
e6bc8b6
Compare
4 tasks
Addresses pre-existing code quality issues identified during clang-tidy review (CodeRabbit findings on PR #225): Critical data corruption / logic bugs: - sd_message: fix IPv4MulticastOption length written to wrong indices (data[2]/data[3] → data[0]/data[1]) - standard_profile: reject duplicate E2E counters to prevent replay attacks (header.counter == last_counter now returns INVALID_ARGUMENT) Concurrency / data race fixes: - freertos/memory: make pool_initialized std::atomic<bool> with proper memory ordering to fix broken double-checked locking pattern - threadx/memory: same fix for pool_initialized and guard_created - tp_manager: collect timed-out transfers under lock, invoke completion_callback_ after releasing transfers_mutex_ - sd_client: copy callbacks from subscriptions/pending_finds maps under their respective locks, invoke them after lock release Type safety / narrowing fixes: - tp_reassembler: widen TP offset from uint16_t to uint32_t (28-bit field × 16 can exceed 64 KB) - serializer: default DeserializationResult error_ to NOT_INITIALIZED instead of SUCCESS to prevent get_value() on empty optional - e2e_crc: add PTRDIFF_MAX overflow guard before size_t → ptrdiff_t cast in iterator arithmetic Closes #226 Made-with: Cursor
Contributor
There was a problem hiding this comment.
Actionable comments posted: 6
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
src/rpc/rpc_client.cpp (1)
80-89:⚠️ Potential issue | 🔴 CriticalExtract callbacks outside
pending_calls_mutex_scope before invoking them to prevent deadlock on re-entrance.All three locations (lines 80-89, 182-193, 225-242) invoke callbacks while holding
pending_calls_mutex_. SinceRpcCallback = std::function<void(const RpcResponse&)>is user-provided and the mutex is non-recursive (std::mutex on POSIX/WIN32, binary semaphore on FreeRTOS, k_mutex on Zephyr, TX_INHERIT on ThreadX), a callback re-enteringcancel_call(),shutdown(), orcall_method_*()will deadlock.Suggested fix pattern
bool cancel_call(RpcCallHandle handle) { - platform::ScopedLock const lock(pending_calls_mutex_); - auto it = pending_calls_.find(handle); - if (it == pending_calls_.end()) { - return false; - } - - // Call callback with cancellation result - if (it->second.callback) { - RpcResponse response(it->second.service_id, it->second.method_id, - client_id_, it->second.session_id, RpcResult::INTERNAL_ERROR); - it->second.callback(response); - } - - pending_calls_.erase(it); - return true; + RpcCallback cb; + RpcResponse response(0, 0, client_id_, 0, RpcResult::INTERNAL_ERROR); + { + platform::ScopedLock const lock(pending_calls_mutex_); + auto it = pending_calls_.find(handle); + if (it == pending_calls_.end()) { + return false; + } + response = RpcResponse(it->second.service_id, it->second.method_id, + client_id_, it->second.session_id, RpcResult::INTERNAL_ERROR); + cb = std::move(it->second.callback); + pending_calls_.erase(it); + } + if (cb) { + cb(response); + } + return true; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/rpc/rpc_client.cpp` around lines 80 - 89, The code currently invokes user-provided RpcCallback while holding pending_calls_mutex_ (see occurrences around the loop that constructs RpcResponse using pending_calls_ and invoking pair.second.callback), which can deadlock if callbacks re-enter cancel_call(), shutdown(), or any call_method_*; fix by collecting the callbacks and their RpcResponse (or minimal call identifiers) into a local vector while holding pending_calls_mutex_, then release the mutex and iterate that vector to invoke each callback; update the three sites (the block using pending_calls_mutex_ at lines ~80-89, and the blocks at ~182-193 and ~225-242) to use this pattern and ensure pending_calls_.clear() (or removal) happens while mutex is held before invoking callbacks.src/tp/tp_reassembler.cpp (1)
45-57:⚠️ Potential issue | 🔴 CriticalCast each payload byte to
uint32_tbefore the left shifts.Lines 52-53 shift promoted
uint8_tvalues asint. When the high byte is>= 0x80, the result exceedsINT_MAX(e.g.,255 << 24 = 4278190080 > 2147483647), triggering undefined behavior per the C++ standard [expr.shift]/2. Widen each byte touint32_tfirst, then composetp_header.Suggested fix
- uint32_t tp_header = (payload[16] << 24) | (payload[17] << 16) | - (payload[18] << 8) | payload[19]; + uint32_t const tp_header = + (static_cast<uint32_t>(payload[16]) << 24U) | + (static_cast<uint32_t>(payload[17]) << 16U) | + (static_cast<uint32_t>(payload[18]) << 8U) | + static_cast<uint32_t>(payload[19]);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/tp/tp_reassembler.cpp` around lines 45 - 57, In TpReassembler::parse_tp_header the expression building tp_header shifts uint8_t values which are promoted to signed int and can trigger undefined behavior for bytes >= 0x80; fix by casting each payload element to uint32_t before shifting (e.g., static_cast<uint32_t>(payload[16]) << 24, etc.) when composing tp_header and ensure any subsequent uses of tp_header (offset_units calculation and offset assignment) operate on the corrected unsigned value.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/e2e/e2e_crc.cpp`:
- Around line 117-119: The calculate_crc function currently returns 0 on invalid
input which is ambiguous with a valid CRC; change its return type to
std::optional<uint32_t> (or a Result-like type) and have calculate_crc return
std::nullopt on error instead of 0 so callers (e.g.,
standard_profile.cpp::validate()) can distinguish an error from a legitimate CRC
value; update all call sites to check the optional before comparing expected vs
received CRC and propagate or log errors appropriately.
In `@src/events/event_subscriber.cpp`:
- Line 261: In on_message_received, the code currently takes
subscriptions_mutex_ then later takes field_requests_mutex_, which is safe now
but risky if future code acquires them in the opposite order; update the
implementation or comments: either (A) change on_message_received (or the helper
that needs field_requests_) to release subscriptions_mutex_ before acquiring
field_requests_mutex_, or (B) add a clear comment near the mutex declarations
and at the top of on_message_received documenting the required lock ordering
("always take subscriptions_mutex_ before field_requests_mutex_") so future
authors preserve the order; reference subscriptions_mutex_,
field_requests_mutex_, and on_message_received when making the change.
In `@src/platform/freertos/memory.cpp`:
- Around line 40-56: The lazy creation of pool_mutex in ensure_pool_init() is
racy because the check if (!pool_mutex) is unsynchronized; replace that ad-hoc
creation with a true one-time init using std::once_flag and std::call_once
(declare a static std::once_flag e.g. pool_mutex_once) and perform
xSemaphoreCreateMutex() inside the call_once lambda so only one task creates
pool_mutex; keep the rest of ensure_pool_init() logic (taking the mutex,
initializing block_used, and setting pool_initialized) intact and ensure
pool_mutex is the mutex created by the call_once, and update any references from
allocate_message() to rely on ensure_pool_init() for safe initialization.
In `@src/platform/threadx/memory.cpp`:
- Around line 45-50: The one-time init using the atomic guard (guard_created) is
racy and ignores ThreadX return values; replace the double-check with
std::call_once (use a static std::once_flag) to ensure tx_mutex_create is
executed exactly once, and validate the return codes from tx_mutex_create,
tx_mutex_get and tx_block_pool_create (and any other ThreadX calls) before
setting the published initialization flag (pool_initialized); only mark the pool
as initialized after all API calls succeed and handle/log failures accordingly
while referencing pool_guard, guard_created (remove), tx_mutex_create,
tx_mutex_get, tx_block_pool_create, and pool_initialized so reviewers can find
the changes.
In `@src/serialization/serializer.cpp`:
- Around line 149-152: Guard against alignment == 0 in Serializer::align_to (and
any other places in this file that do `... % alignment`) to avoid undefined
behavior: add an early check (e.g., if (alignment == 0) return; or assert/throw)
before performing `(current_size % alignment)` so the modulus is never computed
with zero; apply the same guard to the other modulus operation(s) in this file
that use `alignment`.
In `@src/tp/tp_manager.cpp`:
- Around line 189-219: The code reads completion_callback_ without
synchronization in TpManager::process_timeouts(), causing a data race with
set_completion_callback(); fix by taking transfers_mutex_ lock to snapshot
completion_callback_ into a local variable while still inside the locked scope
(before releasing the lock and invoking callbacks), and modify
set_completion_callback() to acquire the same transfers_mutex_ before assigning
to completion_callback_; use the local copy for invoking completion callbacks
outside the lock to preserve the unlock-before-callback behavior.
---
Outside diff comments:
In `@src/rpc/rpc_client.cpp`:
- Around line 80-89: The code currently invokes user-provided RpcCallback while
holding pending_calls_mutex_ (see occurrences around the loop that constructs
RpcResponse using pending_calls_ and invoking pair.second.callback), which can
deadlock if callbacks re-enter cancel_call(), shutdown(), or any call_method_*;
fix by collecting the callbacks and their RpcResponse (or minimal call
identifiers) into a local vector while holding pending_calls_mutex_, then
release the mutex and iterate that vector to invoke each callback; update the
three sites (the block using pending_calls_mutex_ at lines ~80-89, and the
blocks at ~182-193 and ~225-242) to use this pattern and ensure
pending_calls_.clear() (or removal) happens while mutex is held before invoking
callbacks.
In `@src/tp/tp_reassembler.cpp`:
- Around line 45-57: In TpReassembler::parse_tp_header the expression building
tp_header shifts uint8_t values which are promoted to signed int and can trigger
undefined behavior for bytes >= 0x80; fix by casting each payload element to
uint32_t before shifting (e.g., static_cast<uint32_t>(payload[16]) << 24, etc.)
when composing tp_header and ensure any subsequent uses of tp_header
(offset_units calculation and offset assignment) operate on the corrected
unsigned value.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 346f2f42-0390-4cf6-a513-768ebd801919
📒 Files selected for processing (56)
.clang-tidyinclude/e2e/e2e_config.hinclude/e2e/e2e_crc.hinclude/e2e/e2e_header.hinclude/e2e/e2e_profile.hinclude/e2e/e2e_profile_registry.hinclude/e2e/e2e_protection.hinclude/events/event_publisher.hinclude/events/event_subscriber.hinclude/events/event_types.hinclude/platform/host/host_condition_variable.hinclude/platform/posix/memory_impl.hinclude/platform/posix/net_impl.hinclude/platform/posix/thread_impl.hinclude/platform/thread.hinclude/platform/threadx/memory_internal.hinclude/rpc/rpc_client.hinclude/rpc/rpc_server.hinclude/rpc/rpc_types.hinclude/sd/sd_client.hinclude/sd/sd_message.hinclude/sd/sd_server.hinclude/sd/sd_types.hinclude/serialization/serializer.hinclude/tp/tp_manager.hinclude/tp/tp_reassembler.hinclude/tp/tp_segmenter.hinclude/tp/tp_types.hinclude/transport/endpoint.hinclude/transport/tcp_transport.hinclude/transport/transport.hinclude/transport/udp_transport.hsrc/core/session_manager.cppsrc/e2e/e2e_crc.cppsrc/e2e/e2e_header.cppsrc/e2e/e2e_profile_registry.cppsrc/e2e/e2e_profiles/standard_profile.cppsrc/e2e/e2e_protection.cppsrc/events/event_publisher.cppsrc/events/event_subscriber.cppsrc/platform/freertos/memory.cppsrc/platform/threadx/memory.cppsrc/rpc/rpc_client.cppsrc/rpc/rpc_server.cppsrc/sd/sd_client.cppsrc/sd/sd_message.cppsrc/sd/sd_server.cppsrc/serialization/serializer.cppsrc/someip/message.cppsrc/tp/tp_manager.cppsrc/tp/tp_reassembler.cppsrc/tp/tp_segmenter.cppsrc/transport/endpoint.cppsrc/transport/tcp_transport.cppsrc/transport/udp_transport.cpptests/threadx/test_threadx_core.cpp
Fixes test failures and resolves all remaining open review findings: Test fix: - Revert E2E counter equality rejection — protect() and validate() share the same counters_ map, so header.counter == last_counter is expected behavior, not a replay Concurrency fixes: - rpc_client: extract callbacks from pending_calls_mutex_ before invoking in shutdown(), cancel_call(), and on_message_received() - tp_manager: synchronize set_completion/progress/message_callback() under transfers_mutex_; snapshot callback under lock in process_timeouts() before invoking outside - freertos/memory: replace racy double-checked locking with std::atomic_flag spinlock for one-time pool initialization - threadx/memory: same spinlock pattern; remove guard_created flag Safety / correctness fixes: - serializer: guard Serializer::align_to and Deserializer::align_to against zero alignment (UB from % 0) - e2e_crc: change calculate_crc() return type to std::optional<uint32_t> so error cases are distinguishable from valid CRC value 0; update callers in standard_profile.cpp and all tests Closes #226 Made-with: Cursor
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
include/e2e/e2e_crc.h (1)
65-73: 🧹 Nitpick | 🔵 TrivialDocumentation should reflect the new
std::optionalreturn type.The return type change to
std::optional<uint32_t>is the correct fix for the ambiguity where0could be both an error and a valid CRC. However, the@returndocumentation at line 71 still says "CRC value (size depends on crc_type)" without mentioning thatstd::nulloptis returned on invalid inputs.📝 Suggested documentation update
/** * `@brief` Calculate CRC for a specific range of data * `@param` data Data vector * `@param` offset Start offset * `@param` length Length of data to process * `@param` crc_type 0 = SAE-J1850 (8-bit), 1 = ITU-T X.25 (16-bit), 2 = CRC32 - * `@return` CRC value (size depends on crc_type) + * `@return` CRC value (size depends on crc_type), or std::nullopt on invalid inputs + * (out-of-bounds offset/length, unknown crc_type, or values exceeding PTRDIFF_MAX) */🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@include/e2e/e2e_crc.h` around lines 65 - 73, Update the calculate_crc documentation to state that it now returns std::optional<uint32_t> and that std::nullopt is returned for invalid inputs (e.g., out-of-range offset/length or unsupported crc_type); clarify that when set, the uint32_t contains the CRC value whose size depends on crc_type (8/16/32 bits), and mention which crc_type values are supported (0,1,2) and that other values yield std::nullopt.src/e2e/e2e_crc.cpp (1)
78-101:⚠️ Potential issue | 🟡 MinorCRC32 table initialization is not thread-safe.
init_crc32_table()uses a plainbool crc32_table_initializedflag without synchronization. If multiple threads callcalculate_crc32()concurrently for the first time, they may race on writing tocrc32_table[], potentially causing corrupted table entries.This is a pre-existing issue, but given the PR's focus on fixing concurrency bugs in other files (e.g., platform allocators), it may be worth addressing here as well.
🔒 Suggested fix using std::call_once
+#include <mutex> + // Precomputed CRC32 lookup table static uint32_t crc32_table[256]; - -// Initialize CRC32 lookup table (called once) -static bool crc32_table_initialized = false; +static std::once_flag crc32_table_once; static void init_crc32_table() { - if (crc32_table_initialized) { - return; - } - - for (uint32_t i = 0; i < 256; ++i) { - uint32_t crc = i << 24; - for (int j = 0; j < 8; ++j) { - if (crc & 0x80000000) { - crc = (crc << 1) ^ CRC32_POLY; - } else { - crc <<= 1; + std::call_once(crc32_table_once, []() { + for (uint32_t i = 0; i < 256; ++i) { + uint32_t crc = i << 24; + for (int j = 0; j < 8; ++j) { + if (crc & 0x80000000) { + crc = (crc << 1) ^ CRC32_POLY; + } else { + crc <<= 1; + } } + crc32_table[i] = crc; } - crc32_table[i] = crc; - } - - crc32_table_initialized = true; + }); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/e2e/e2e_crc.cpp` around lines 78 - 101, The crc32 table initialization in init_crc32_table() is not thread-safe; replace the manual bool crc32_table_initialized check with a thread-safe one-time initialization (use std::call_once and a static std::once_flag) so multiple threads cannot concurrently populate crc32_table; keep the table population logic inside the call_once-protected function and remove or repurpose crc32_table_initialized, referencing init_crc32_table, crc32_table, and any callers like calculate_crc32 to ensure they trigger the call_once initialization.tests/test_e2e.cpp (1)
286-299:⚠️ Potential issue | 🔴 CriticalType mismatch: test assigns
std::optional<uint32_t>touint32_tvariables.Lines 289-292 assign the result of
calculate_crc()(nowstd::optional<uint32_t>) directly touint32_tvariables. This will fail to compile sincestd::optionaldoes not implicitly convert to its contained type. Additionally, line 298 comparesunkagainst0u, butunkis now anstd::optionaland should be checked with!has_value().🐛 Proposed fix
TEST_F(E2ETest, CRC_AllTypeBranches) { std::vector<uint8_t> data = {0x01, 0x02, 0x03, 0x04}; - uint32_t crc8 = E2ECRC::calculate_crc(data, 0, 4, 0); - uint32_t crc16 = E2ECRC::calculate_crc(data, 0, 4, 1); - uint32_t crc32 = E2ECRC::calculate_crc(data, 0, 4, 2); - uint32_t unk = E2ECRC::calculate_crc(data, 0, 4, 255); + auto crc8 = E2ECRC::calculate_crc(data, 0, 4, 0); + auto crc16 = E2ECRC::calculate_crc(data, 0, 4, 1); + auto crc32 = E2ECRC::calculate_crc(data, 0, 4, 2); + auto unk = E2ECRC::calculate_crc(data, 0, 4, 255); // Ranged CRC should match direct calls - EXPECT_EQ(crc8, static_cast<uint32_t>(E2ECRC::calculate_crc8_sae_j1850(data))); - EXPECT_EQ(crc16, static_cast<uint32_t>(E2ECRC::calculate_crc16_itu_x25(data))); - EXPECT_EQ(crc32, E2ECRC::calculate_crc32(data)); - EXPECT_EQ(unk, 0u); // Unknown type returns 0 + ASSERT_TRUE(crc8.has_value()); + ASSERT_TRUE(crc16.has_value()); + ASSERT_TRUE(crc32.has_value()); + EXPECT_EQ(crc8.value(), static_cast<uint32_t>(E2ECRC::calculate_crc8_sae_j1850(data))); + EXPECT_EQ(crc16.value(), static_cast<uint32_t>(E2ECRC::calculate_crc16_itu_x25(data))); + EXPECT_EQ(crc32.value(), E2ECRC::calculate_crc32(data)); + EXPECT_FALSE(unk.has_value()); // Unknown type returns nullopt }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/test_e2e.cpp` around lines 286 - 299, The test assigns the return of E2ECRC::calculate_crc (now returning std::optional<uint32_t>) to plain uint32_t variables and compares an optional to 0; update the test to store results in std::optional<uint32_t> (e.g., crc8, crc16, crc32, unk as std::optional<uint32_t>), assert that the known CRC optionals have values (use has_value()/operator bool) and compare their contained values via .value() to the direct-call results, and assert that unk has no value (use !unk.has_value() or equivalent) instead of comparing it to 0u; reference calculate_crc and the direct comparators calculate_crc8_sae_j1850, calculate_crc16_itu_x25, and calculate_crc32 to locate the checks.
♻️ Duplicate comments (2)
src/platform/threadx/memory.cpp (1)
48-56:⚠️ Potential issue | 🟠 MajorThreadX API return values are unchecked.
tx_mutex_create()andtx_block_pool_create()both returnUINTstatus codes that should be validated. If either fails,pool_initializedis still set totrue, which will cause subsequenttx_block_allocate()calls to operate on an uninitialized pool—leading to undefined behavior or crashes.🛡️ Proposed fix to validate return codes
if (!pool_initialized.load(std::memory_order_relaxed)) { - tx_mutex_create(&pool_guard, const_cast<CHAR*>("someip_pool_guard"), - TX_NO_INHERIT); - tx_block_pool_create(&message_pool, - const_cast<CHAR*>("someip_msg"), - sizeof(someip::Message), - pool_buffer, - sizeof(pool_buffer)); - pool_initialized.store(true, std::memory_order_release); + UINT status = tx_mutex_create(&pool_guard, const_cast<CHAR*>("someip_pool_guard"), + TX_NO_INHERIT); + if (status == TX_SUCCESS) { + status = tx_block_pool_create(&message_pool, + const_cast<CHAR*>("someip_msg"), + sizeof(someip::Message), + pool_buffer, + sizeof(pool_buffer)); + } + if (status == TX_SUCCESS) { + pool_initialized.store(true, std::memory_order_release); + } + // If init failed, pool_initialized remains false; subsequent calls will retry. }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/platform/threadx/memory.cpp` around lines 48 - 56, Check the UINT return values from tx_mutex_create and tx_block_pool_create (compare against TX_SUCCESS) before setting pool_initialized; if either call fails, log or handle the error, undo any partially-created resource (e.g., delete the mutex if pool creation fails), and do not set pool_initialized to true so that subsequent tx_block_allocate calls won't use an uninitialized pool. Update the initialization block around tx_mutex_create, tx_block_pool_create, pool_initialized, and any cleanup paths to validate return codes and perform proper teardown on failure.src/serialization/serializer.cpp (1)
403-405:⚠️ Potential issue | 🟠 MajorAdd a
PTRDIFF_MAXguard before narrowing casts in iterator arithmetic.
position_(size_t) andlength(uint32_t) are narrowed tostd::ptrdiff_twithout an explicit representability check. On systems where size_t exceeds ptrdiff_t width, values larger thanPTRDIFF_MAXwill cause undefined behavior in iterator arithmetic. The codebase already demonstrates this guard pattern insrc/e2e/e2e_crc.cpp(lines 118–119).Proposed fix
`#include` "serialization/serializer.h" `#include` <cstring> `#include` <algorithm> +#include <limits> `#include` "platform/byteorder.h" @@ DeserializationResult<std::string> Deserializer::deserialize_string() { @@ - if (position_ + length > buffer_.size()) { + if (length > buffer_.size() - position_) { return DeserializationResult<std::string>::error(Result::MALFORMED_MESSAGE); } - auto first = buffer_.begin() + static_cast<std::ptrdiff_t>(position_); - std::string result(first, first + static_cast<std::ptrdiff_t>(length)); + constexpr auto kMaxDiff = static_cast<size_t>(std::numeric_limits<std::ptrdiff_t>::max()); + const size_t length_sz = static_cast<size_t>(length); + if (position_ > kMaxDiff || length_sz > (kMaxDiff - position_)) { + return DeserializationResult<std::string>::error(Result::MALFORMED_MESSAGE); + } + + auto first = buffer_.begin() + static_cast<std::ptrdiff_t>(position_); + std::string result(first, first + static_cast<std::ptrdiff_t>(length_sz));🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/serialization/serializer.cpp` around lines 403 - 405, Before performing iterator arithmetic that casts position_ and length to std::ptrdiff_t in the serializer code (the block that computes "auto first = buffer_.begin() + static_cast<std::ptrdiff_t>(position_);" and constructs result from that range), add a guard that checks both position_ and length do not exceed PTRDIFF_MAX (and that position_ + length <= PTRDIFF_MAX) before narrowing; if the check fails, return or throw a clear error/exception (consistent with surrounding error handling) instead of performing the cast. Ensure the check references the existing members/variables position_, length and buffer_ and mirrors the PTRDIFF_MAX pattern used in e2e_crc.cpp so iterator arithmetic uses only representable ptrdiff_t values.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/platform/freertos/memory.cpp`:
- Around line 48-55: The mutex allocation can fail and configASSERT is a no-op
in release builds, so after calling xSemaphoreCreateMutex() you must explicitly
check pool_mutex for nullptr and handle the failure instead of setting
pool_initialized to true; update the initialization block around pool_mutex,
pool_initialized.store(...) and init_lock.clear() so that if
xSemaphoreCreateMutex() returns nullptr you do not mark the pool as initialized
(and optionally report/log the error or return a failure), leaving
pool_initialized false to prevent allocate_message() from calling
xSemaphoreTake(nullptr,...); ensure the check references pool_mutex and
functions xSemaphoreCreateMutex, pool_initialized.store, init_lock.clear and
that allocate_message will only proceed when pool_initialized is true.
In `@src/rpc/rpc_client.cpp`:
- Around line 80-95: The callbacks in shutdown/erase paths (code around
pending_calls_ handling that builds shutdown_cbs and later invokes cb(resp)) can
outlive stack-local synchronization state used by call_method_sync() (e.g.,
sync_mtx, sync_resp, response_ready), causing use-after-free; change the sync
completion state to shared ownership (e.g., wrap the mutex, response storage and
readiness flag in a shared_ptr/struct) and store that shared state in the
RpcCallback capture so the callback writes into heap-owned state, or
alternatively ensure the pending entry is not erased (or block until cb
finishes) before call_method_sync() returns; update locations that build
RpcResponse and invoke RpcCallback (the shutdown_cbs creation and the similar
blocks at the other noted ranges) to capture or wait on the shared state so
cancel_call(handle) and call_method_sync() cannot return while a queued callback
may access stack locals.
In `@src/tp/tp_manager.cpp`:
- Around line 192-227: Hoist the invariant null-check on the captured completion
callback to avoid repeated evaluation: in TpManager::process_timeouts keep
capturing completion_callback_ into local cb and building the timed_out vector
as-is, but move the if (cb) check outside the for-loop that iterates over
timed_out so you only test once before invoking cb(id, result); this touches the
local variable cb, the timed_out collection and the invocation loop.
---
Outside diff comments:
In `@include/e2e/e2e_crc.h`:
- Around line 65-73: Update the calculate_crc documentation to state that it now
returns std::optional<uint32_t> and that std::nullopt is returned for invalid
inputs (e.g., out-of-range offset/length or unsupported crc_type); clarify that
when set, the uint32_t contains the CRC value whose size depends on crc_type
(8/16/32 bits), and mention which crc_type values are supported (0,1,2) and that
other values yield std::nullopt.
In `@src/e2e/e2e_crc.cpp`:
- Around line 78-101: The crc32 table initialization in init_crc32_table() is
not thread-safe; replace the manual bool crc32_table_initialized check with a
thread-safe one-time initialization (use std::call_once and a static
std::once_flag) so multiple threads cannot concurrently populate crc32_table;
keep the table population logic inside the call_once-protected function and
remove or repurpose crc32_table_initialized, referencing init_crc32_table,
crc32_table, and any callers like calculate_crc32 to ensure they trigger the
call_once initialization.
In `@tests/test_e2e.cpp`:
- Around line 286-299: The test assigns the return of E2ECRC::calculate_crc (now
returning std::optional<uint32_t>) to plain uint32_t variables and compares an
optional to 0; update the test to store results in std::optional<uint32_t>
(e.g., crc8, crc16, crc32, unk as std::optional<uint32_t>), assert that the
known CRC optionals have values (use has_value()/operator bool) and compare
their contained values via .value() to the direct-call results, and assert that
unk has no value (use !unk.has_value() or equivalent) instead of comparing it to
0u; reference calculate_crc and the direct comparators calculate_crc8_sae_j1850,
calculate_crc16_itu_x25, and calculate_crc32 to locate the checks.
---
Duplicate comments:
In `@src/platform/threadx/memory.cpp`:
- Around line 48-56: Check the UINT return values from tx_mutex_create and
tx_block_pool_create (compare against TX_SUCCESS) before setting
pool_initialized; if either call fails, log or handle the error, undo any
partially-created resource (e.g., delete the mutex if pool creation fails), and
do not set pool_initialized to true so that subsequent tx_block_allocate calls
won't use an uninitialized pool. Update the initialization block around
tx_mutex_create, tx_block_pool_create, pool_initialized, and any cleanup paths
to validate return codes and perform proper teardown on failure.
In `@src/serialization/serializer.cpp`:
- Around line 403-405: Before performing iterator arithmetic that casts
position_ and length to std::ptrdiff_t in the serializer code (the block that
computes "auto first = buffer_.begin() +
static_cast<std::ptrdiff_t>(position_);" and constructs result from that range),
add a guard that checks both position_ and length do not exceed PTRDIFF_MAX (and
that position_ + length <= PTRDIFF_MAX) before narrowing; if the check fails,
return or throw a clear error/exception (consistent with surrounding error
handling) instead of performing the cast. Ensure the check references the
existing members/variables position_, length and buffer_ and mirrors the
PTRDIFF_MAX pattern used in e2e_crc.cpp so iterator arithmetic uses only
representable ptrdiff_t values.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: eed814be-e183-49d2-9ba6-895cde467afd
📒 Files selected for processing (10)
include/e2e/e2e_crc.hsrc/e2e/e2e_crc.cppsrc/e2e/e2e_profiles/standard_profile.cppsrc/platform/freertos/memory.cppsrc/platform/threadx/memory.cppsrc/rpc/rpc_client.cppsrc/serialization/serializer.cppsrc/tp/tp_manager.cpptests/shared/test_e2e_common.inctests/test_e2e.cpp
CI fix: - test_e2e: fix CRC_AllTypeBranches test that was still using uint32_t instead of auto for std::optional<uint32_t> return type Review findings: - freertos/memory: guard pool_initialized against mutex creation failure — configASSERT is a no-op in release builds, so also check pool_mutex != nullptr before marking initialized - threadx/memory: check tx_mutex_create and tx_block_pool_create return status; bail out without marking initialized on failure - rpc_client: fix use-after-free in call_method_sync — move sync state (mutex, response, ready flag) to shared_ptr so the callback closure outlives the stack frame if cancel_call races - tp_manager: hoist invariant if(cb) check outside the loop - event_subscriber: document lock ordering (subscriptions_mutex_ before field_requests_mutex_) to prevent future deadlocks Made-with: Cursor
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
src/events/event_subscriber.cpp (2)
261-279:⚠️ Potential issue | 🔴 CriticalMove user callback invocation out of internal mutex critical sections.
Line 261/278 and Line 290/299 invoke user-provided callbacks while
subscriptions_mutex_/field_requests_mutex_are held. This can deadlock on re-entrancy (e.g., callback callsunsubscribe_eventgroup()orrequest_field()) and stalls message handling.🔧 Suggested fix (two-phase: capture under lock, invoke after unlock)
void on_message_received(MessagePtr message, const transport::Endpoint& /*sender*/) override { @@ - platform::ScopedLock const subs_lock(subscriptions_mutex_); + EventNotificationCallback notification_cb{}; + EventNotification notification(0, 0, 0); + bool has_notification = false; + { + platform::ScopedLock const subs_lock(subscriptions_mutex_); @@ - if (sub_info.notification_callback) { - sub_info.notification_callback(notification); - } + notification_cb = sub_info.notification_callback; + has_notification = static_cast<bool>(notification_cb); @@ - break; + break; } } + } + + if (has_notification) { + notification_cb(notification); + } @@ - platform::ScopedLock const field_lock(field_requests_mutex_); - std::string field_key = make_field_key(service_id, 0, event_id); // Simplified - - auto field_it = field_requests_.find(field_key); - if (field_it != field_requests_.end()) { - EventNotification notification(service_id, 0, event_id); - notification.event_data = message->get_payload(); - - if (field_it->second) { - field_it->second(notification); - } - - field_requests_.erase(field_it); - } + EventNotificationCallback field_cb{}; + EventNotification field_notification(service_id, 0, event_id); + bool has_field_cb = false; + { + platform::ScopedLock const field_lock(field_requests_mutex_); + std::string field_key = make_field_key(service_id, 0, event_id); // Simplified + auto field_it = field_requests_.find(field_key); + if (field_it != field_requests_.end()) { + field_notification.event_data = message->get_payload(); + field_cb = field_it->second; + has_field_cb = static_cast<bool>(field_cb); + field_requests_.erase(field_it); + } + } + if (has_field_cb) { + field_cb(field_notification); + } }Also applies to: 290-300
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/events/event_subscriber.cpp` around lines 261 - 279, The code is calling user callbacks while holding internal locks (subscriptions_mutex_ and field_requests_mutex_), which can deadlock on re-entrancy; fix by doing a two-phase approach: inside the critical section (in the loop over subscriptions_ and in the field_requests_ handling) capture all necessary data into local variables/objects (e.g., construct EventNotification, copy notification.client_id/session_id/event_data and store the std::function callback references like sub_info.notification_callback or field_request_callback) and then release the mutex before invoking the callbacks; update the blocks that create EventNotification and call sub_info.notification_callback as well as the field request path that calls field_request_callback to follow this capture-then-unlock-then-invoke pattern.
71-75:⚠️ Potential issue | 🟠 MajorReduce lock scopes around transport calls to avoid blocking receive thread.
Transport calls (
send_message(),stop()) are executed while holdingsubscriptions_mutex_(lines 71, 98–123, 131–159) orfield_requests_mutex_(line 170–188). Since the transport's receive thread invokes listener callbacks (on_message_received(),on_connection_lost()) that contend for these same locks, holding them during network I/O creates unnecessary blocking and lock contention. Collect data under lock, then release before calling transport; reacquire only for post-send cleanup if needed.Also applies to: 98–123, 131–159, 170–188
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/events/event_subscriber.cpp` around lines 71 - 75, Current code holds subscriptions_mutex_ or field_requests_mutex_ while calling transport methods (e.g., transport_->stop(), send_message()), which blocks the transport receive thread callbacks (on_message_received(), on_connection_lost()); change each such site to: acquire the mutex, copy or move out only the minimal data needed for the transport call (e.g., list of subscriptions or message payload), release the mutex, perform transport_->stop() or send_message(), and then re-acquire the mutex only if you must perform post-call cleanup or mutation of subscriptions_ / field_requests_. Ensure this pattern is applied to the blocks touching subscriptions_mutex_ (around subscriptions_.clear() and the send_message sites) and field_requests_mutex_ (lines handling field requests) to avoid holding locks during network I/O.
♻️ Duplicate comments (2)
src/platform/freertos/memory.cpp (1)
48-54:⚠️ Potential issue | 🔴 CriticalPropagate pool-init failure before taking
pool_mutex.If
xSemaphoreCreateMutex()returnsnullptr, Lines 51-54 correctly avoid publishingpool_initialized, butallocate_message()still callsxSemaphoreTake(pool_mutex, ...)on Line 66. In release builds whereconfigASSERTis compiled out, that turns an init failure into a null-handle dereference. Makeensure_pool_init()return success/failure and bail out here when initialization did not complete.Proposed fix
-static void ensure_pool_init() { +static bool ensure_pool_init() { if (pool_initialized.load(std::memory_order_acquire)) { - return; + return true; } while (init_lock.test_and_set(std::memory_order_acquire)) { } if (!pool_initialized.load(std::memory_order_relaxed)) { pool_mutex = xSemaphoreCreateMutex(); configASSERT(pool_mutex != nullptr); if (pool_mutex != nullptr) { std::memset(block_used, 0, sizeof(block_used)); pool_initialized.store(true, std::memory_order_release); } } + bool const ready = pool_initialized.load(std::memory_order_acquire); init_lock.clear(std::memory_order_release); + return ready; } /** `@implements` REQ_PLATFORM_FREERTOS_002 */ MessagePtr allocate_message() { - ensure_pool_init(); + if (!ensure_pool_init()) { + return nullptr; + } xSemaphoreTake(pool_mutex, portMAX_DELAY);Also applies to: 63-66
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/platform/freertos/memory.cpp` around lines 48 - 54, The pool init can fail (xSemaphoreCreateMutex may return nullptr) but ensure_pool_init currently doesn't signal failure, causing callers like allocate_message() to call xSemaphoreTake(pool_mutex, ...) and potentially dereference nullptr; change ensure_pool_init() to return a bool (or error code) indicating success/failure, set pool_initialized only on success, and update all callers (notably allocate_message() and any code paths around the second init block at the other site) to check the return value and bail out when initialization failed instead of taking/using pool_mutex; ensure any early returns maintain thread-safety and do not rely on pool_mutex if it is null.src/platform/threadx/memory.cpp (1)
48-65:⚠️ Potential issue | 🔴 CriticalPartial init failures still leave
allocate_message()on an invalid path.
ensure_pool_init()now checkstx_mutex_create()andtx_block_pool_create(), but it still returnsvoid, so Line 78 runs even whenmessage_poolwas never created. There is a second bug here too: iftx_mutex_create()succeeds once andtx_block_pool_create()fails, the next retry will calltx_mutex_create()again on the already-createdpool_guard. Please return success/failure fromensure_pool_init()and track guard creation separately from pool readiness.Also applies to: 74-79
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/platform/threadx/memory.cpp` around lines 48 - 65, ensure_pool_init() currently returns void which allows allocate_message() to proceed even when message_pool wasn't created; change ensure_pool_init() to return bool, introduce a separate atomic<bool> guard_initialized (or similar) to track whether pool_guard was successfully created, and only call tx_mutex_create() when guard_initialized is false; on tx_block_pool_create() failure, if the mutex was just created either destroy it or leave guard_initialized true but ensure pool_initialized stays false and return false; callers like allocate_message() must check the new boolean result and avoid using message_pool when ensure_pool_init() returns false; update references to pool_initialized, init_lock, pool_guard, message_pool, tx_mutex_create, and tx_block_pool_create accordingly.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In @.gitignore:
- Line 103: Move the gitignore entry "build_lib/" out of the file end and place
it into the existing "Build directories" section alongside the other build
patterns (e.g., "build/", "build_test/", "build-*/") so all build-related
entries are grouped; locate the "Build directories" block and add "build_lib/"
there, then remove the duplicate entry at the end of the file.
In `@src/platform/freertos/memory.cpp`:
- Around line 46-57: The current busy-spin using init_lock.test_and_set causes
starvation in FreeRTOS; replace this with a blocking one-time init using a
FreeRTOS synchronization primitive (e.g., a binary semaphore or mutex) so tasks
call xSemaphoreTake to wait instead of spinning, and perform initialization only
when pool_initialized is false; specifically modify the init sequence around
init_lock, pool_initialized and pool_mutex so callers of allocate_message()
block on the semaphore until initialization completes. Also handle
xSemaphoreCreateMutex() failure: if creating pool_mutex fails do not leave
pool_mutex nullptr—either retry creation, set an explicit initialization-failed
flag and return an error to callers of allocate_message(), or assert/propagate
the error so subsequent xSemaphoreTake(pool_mutex, ...) is never called on a
null pointer; ensure block_used is only memset after successful mutex creation
and pool_initialized.store(true) only after that.
In `@src/platform/threadx/memory.cpp`:
- Around line 46-68: Replace the busy-spin on init_lock with a blocking
synchronization primitive (e.g., create and wait on a ThreadX semaphore or use
tx_event_flags) so threads block instead of busy-waiting while initialization is
in progress; specifically change the loop that uses init_lock.test_and_set() to
acquire a tx_semaphore (or equivalent) and release it when initialization
completes. Move the pool setup (tx_mutex_create(&pool_guard,...),
tx_block_pool_create(&message_pool,...)) out of hot allocation paths if
possible, or ensure the init path is protected by the blocking primitive so only
one initializer runs. Add proper rollback on failure: if tx_block_pool_create
fails after tx_mutex_create succeeded, call tx_mutex_delete(&pool_guard) (or
handle TX_ALREADY_CREATED) and clear pool_initialized so subsequent attempts can
retry cleanly. Ensure pool_initialized (std::atomic) is set only after both
tx_mutex_create and tx_block_pool_create succeed and release the blocking
primitive on all code paths.
---
Outside diff comments:
In `@src/events/event_subscriber.cpp`:
- Around line 261-279: The code is calling user callbacks while holding internal
locks (subscriptions_mutex_ and field_requests_mutex_), which can deadlock on
re-entrancy; fix by doing a two-phase approach: inside the critical section (in
the loop over subscriptions_ and in the field_requests_ handling) capture all
necessary data into local variables/objects (e.g., construct EventNotification,
copy notification.client_id/session_id/event_data and store the std::function
callback references like sub_info.notification_callback or
field_request_callback) and then release the mutex before invoking the
callbacks; update the blocks that create EventNotification and call
sub_info.notification_callback as well as the field request path that calls
field_request_callback to follow this capture-then-unlock-then-invoke pattern.
- Around line 71-75: Current code holds subscriptions_mutex_ or
field_requests_mutex_ while calling transport methods (e.g., transport_->stop(),
send_message()), which blocks the transport receive thread callbacks
(on_message_received(), on_connection_lost()); change each such site to: acquire
the mutex, copy or move out only the minimal data needed for the transport call
(e.g., list of subscriptions or message payload), release the mutex, perform
transport_->stop() or send_message(), and then re-acquire the mutex only if you
must perform post-call cleanup or mutation of subscriptions_ / field_requests_.
Ensure this pattern is applied to the blocks touching subscriptions_mutex_
(around subscriptions_.clear() and the send_message sites) and
field_requests_mutex_ (lines handling field requests) to avoid holding locks
during network I/O.
---
Duplicate comments:
In `@src/platform/freertos/memory.cpp`:
- Around line 48-54: The pool init can fail (xSemaphoreCreateMutex may return
nullptr) but ensure_pool_init currently doesn't signal failure, causing callers
like allocate_message() to call xSemaphoreTake(pool_mutex, ...) and potentially
dereference nullptr; change ensure_pool_init() to return a bool (or error code)
indicating success/failure, set pool_initialized only on success, and update all
callers (notably allocate_message() and any code paths around the second init
block at the other site) to check the return value and bail out when
initialization failed instead of taking/using pool_mutex; ensure any early
returns maintain thread-safety and do not rely on pool_mutex if it is null.
In `@src/platform/threadx/memory.cpp`:
- Around line 48-65: ensure_pool_init() currently returns void which allows
allocate_message() to proceed even when message_pool wasn't created; change
ensure_pool_init() to return bool, introduce a separate atomic<bool>
guard_initialized (or similar) to track whether pool_guard was successfully
created, and only call tx_mutex_create() when guard_initialized is false; on
tx_block_pool_create() failure, if the mutex was just created either destroy it
or leave guard_initialized true but ensure pool_initialized stays false and
return false; callers like allocate_message() must check the new boolean result
and avoid using message_pool when ensure_pool_init() returns false; update
references to pool_initialized, init_lock, pool_guard, message_pool,
tx_mutex_create, and tx_block_pool_create accordingly.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: f71d54b1-0e50-47c4-b427-c450c84fa866
📒 Files selected for processing (7)
.gitignoresrc/events/event_subscriber.cppsrc/platform/freertos/memory.cppsrc/platform/threadx/memory.cppsrc/rpc/rpc_client.cppsrc/tp/tp_manager.cpptests/test_e2e.cpp
…tors - freertos/memory: replace std::atomic_flag spin with taskENTER_CRITICAL/taskEXIT_CRITICAL — prevents priority-inversion starvation in preemptive scheduling - threadx/memory: replace spin with TX_DISABLE/TX_RESTORE for consistency - .gitignore: move build_lib/ to Build directories section Made-with: Cursor
If tx_mutex_create succeeds but tx_block_pool_create fails, delete the already-created mutex so subsequent retries start from a clean state instead of permanently failing with TX_MUTEX_ERROR. Ref: #226 Made-with: Cursor
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
First batch of clang-tidy remediation for #222. Reduces violations by ~325 (18% reduction).
.clang-tidyused invalidsnake_caseoption values (should belower_case), silently disablingreadability-identifier-namingchecksbugprone-*(28),cppcoreguidelines-init-variables(19), andcppcoreguidelines-pro-type-member-init(26) violations eliminated:static_cast<ptrdiff_t>for iterator arithmeticRpcClientImpldestructor wrapped in noexcept try-catch for exception safetythread_impl.hjustified with NOLINTstandard_profile.cppcounter validationmodernize-concat-nested-namespaces(147 → 0): C++17namespace foo::barsyntaxmisc-const-correctness(304 → ~160):constadded to non-mutated localsreadability-braces-around-statements(36 → 0): braces on bare if/elsemodernize-use-override,readability-static-accessed-through-instance,performance-avoid-endl, and other small fixesPre-existing issues fixed (#226)
Addresses 9 pre-existing code quality issues flagged by CodeRabbit during review:
IPv4MulticastOption::serialize()wrote length to wrong byte indices; E2E counter validation accepted duplicate counters (replay risk)pool_initializeddata race in FreeRTOS/ThreadX memory allocators (broken DCLP); callbacks invoked under mutexes inTpManager::process_timeouts()andSdClientoffer handling (deadlock risk)uint32_ttouint16_t;DeserializationResultdefault-constructed as success with empty optional; missingPTRDIFF_MAXoverflow guard in CRC calculationNote: Fixing the
.clang-tidyconfig correctly activatesreadability-identifier-naming, surfacing ~165 new naming violations that were previously hidden. These are tracked for a future batch.Remaining violations for subsequent batches
Test plan
cmake --build build)Closes #226
Summary by CodeRabbit
Refactor
Public API changes
Chores