From 677272164b3b80388843bf096cd1407efcc09e4a Mon Sep 17 00:00:00 2001 From: Vinicius Zein Date: Mon, 13 Apr 2026 03:48:43 -0400 Subject: [PATCH 1/3] fix: remediate batch 2 clang-tidy violations Performance: - Use std::move for callback parameters instead of copying - Replace std::endl with '\n' to avoid unnecessary flushes - Pass MessagePtr by const ref where only read access is needed Readability: - Access static members via class name, not instance - Simplify boolean returns in has_valid_method_id/has_valid_length - Remove redundant member initializer in UdpConfig - Use const auto* for pointer-to-const auto declarations - Initialize TpTransfer::last_activity in member initializer list Misc: - Move file-local static variables/functions to anonymous namespaces - Replace C-style casts with reinterpret_cast in socket code Tests: - Add boundary tests for method ID and length validation - Add E2E header size static access test - Add TpTransfer timestamp initialization test Made-with: Cursor --- include/someip/message.h | 2 +- include/tp/tp_types.h | 8 ++--- include/transport/udp_transport.h | 2 +- src/e2e/e2e_crc.cpp | 10 ++++-- src/events/event_subscriber.cpp | 11 ++++--- src/platform/freertos/memory.cpp | 48 +++++++++++++++------------ src/platform/threadx/memory.cpp | 31 +++++++++++------- src/rpc/rpc_client.cpp | 4 +-- src/rpc/rpc_server.cpp | 9 +++--- src/sd/sd_client.cpp | 8 +++-- src/sd/sd_message.cpp | 4 +-- src/sd/sd_server.cpp | 8 +++-- src/someip/message.cpp | 23 ++++--------- src/tp/tp_reassembler.cpp | 2 +- src/transport/tcp_transport.cpp | 10 +++--- tests/test_message.cpp | 54 +++++++++++++++++++++++++++++++ tests/test_tp.cpp | 16 +++++++++ 17 files changed, 172 insertions(+), 78 deletions(-) diff --git a/include/someip/message.h b/include/someip/message.h index f9a34b233f..bcc56ef2a1 100644 --- a/include/someip/message.h +++ b/include/someip/message.h @@ -130,7 +130,7 @@ class Message { // Utility methods size_t get_total_size() const { - size_t e2e_size = e2e_header_.has_value() ? e2e_header_->get_header_size() : 0; + size_t e2e_size = e2e_header_.has_value() ? e2e::E2EHeader::get_header_size() : 0; return HEADER_SIZE + e2e_size + payload_.size(); } static size_t get_header_size() { return HEADER_SIZE; } diff --git a/include/tp/tp_types.h b/include/tp/tp_types.h index 6ab45a8ce9..f5a075e6b4 100644 --- a/include/tp/tp_types.h +++ b/include/tp/tp_types.h @@ -141,10 +141,10 @@ struct TpTransfer { TpTransfer() = default; TpTransfer(uint32_t id, uint32_t msg_id) - : transfer_id(id), message_id(msg_id) { - start_time = std::chrono::steady_clock::now(); - last_activity = start_time; - } + : transfer_id(id), + message_id(msg_id), + start_time(std::chrono::steady_clock::now()), + last_activity(start_time) {} }; /** diff --git a/include/transport/udp_transport.h b/include/transport/udp_transport.h index 79aceff5e7..9baa4b2179 100644 --- a/include/transport/udp_transport.h +++ b/include/transport/udp_transport.h @@ -34,7 +34,7 @@ struct UdpTransportConfig { bool reuse_address{true}; // Allow address reuse (SO_REUSEADDR) bool reuse_port{false}; // Allow port reuse (SO_REUSEPORT) - for multicast bool enable_broadcast{false}; // Enable broadcast sending - std::string multicast_interface{}; // Interface for multicast (empty = INADDR_ANY) + std::string multicast_interface; // Interface for multicast (empty = INADDR_ANY) int multicast_ttl{1}; // Multicast TTL (1 = local network only) // SOME/IP spec recommends max 1400 bytes to avoid IP fragmentation diff --git a/src/e2e/e2e_crc.cpp b/src/e2e/e2e_crc.cpp index 62e2175b84..eadaeb571e 100644 --- a/src/e2e/e2e_crc.cpp +++ b/src/e2e/e2e_crc.cpp @@ -74,13 +74,15 @@ uint16_t calculate_crc16_itu_x25(const std::vector& data) { static constexpr uint32_t CRC32_POLY = 0x04C11DB7; static constexpr uint32_t CRC32_INIT = 0xFFFFFFFF; +namespace { + // Precomputed CRC32 lookup table -static uint32_t crc32_table[256]; +uint32_t crc32_table[256]; // Initialize CRC32 lookup table (called once) -static bool crc32_table_initialized = false; +bool crc32_table_initialized = false; -static void init_crc32_table() { +void init_crc32_table() { if (crc32_table_initialized) { return; } @@ -100,6 +102,8 @@ static void init_crc32_table() { crc32_table_initialized = true; } +} // namespace + uint32_t calculate_crc32(const std::vector& data) { init_crc32_table(); diff --git a/src/events/event_subscriber.cpp b/src/events/event_subscriber.cpp index 278daa2fa6..a09ff47d3b 100644 --- a/src/events/event_subscriber.cpp +++ b/src/events/event_subscriber.cpp @@ -21,6 +21,7 @@ #include #include #include +#include namespace someip::events { @@ -90,8 +91,8 @@ class EventSubscriberImpl : public transport::ITransportListener { SubscriptionInfo sub_info; sub_info.subscription = subscription; - sub_info.notification_callback = notification_callback; - sub_info.status_callback = status_callback; + sub_info.notification_callback = std::move(notification_callback); + sub_info.status_callback = std::move(status_callback); sub_info.filters = filters; // Store subscription @@ -169,7 +170,7 @@ class EventSubscriberImpl : public transport::ITransportListener { // Store callback for field response platform::ScopedLock const field_lock(field_requests_mutex_); std::string key = make_field_key(service_id, instance_id, event_id); - field_requests_[key] = callback; + field_requests_[key] = std::move(callback); // Send field request transport::Endpoint service_endpoint("127.0.0.1", 30500); // TODO: Get from SD @@ -356,7 +357,7 @@ bool EventSubscriber::subscribe_eventgroup(uint16_t service_id, uint16_t instanc SubscriptionStatusCallback status_callback, const std::vector& filters) { return impl_->subscribe_eventgroup(service_id, instance_id, eventgroup_id, - notification_callback, status_callback, filters); + std::move(notification_callback), std::move(status_callback), filters); } bool EventSubscriber::unsubscribe_eventgroup(uint16_t service_id, uint16_t instance_id, uint16_t eventgroup_id) { @@ -365,7 +366,7 @@ bool EventSubscriber::unsubscribe_eventgroup(uint16_t service_id, uint16_t insta bool EventSubscriber::request_field(uint16_t service_id, uint16_t instance_id, uint16_t event_id, EventNotificationCallback callback) { - return impl_->request_field(service_id, instance_id, event_id, callback); + return impl_->request_field(service_id, instance_id, event_id, std::move(callback)); } bool EventSubscriber::set_event_filters(uint16_t service_id, uint16_t instance_id, uint16_t eventgroup_id, diff --git a/src/platform/freertos/memory.cpp b/src/platform/freertos/memory.cpp index 94498ce253..31f3c34fb7 100644 --- a/src/platform/freertos/memory.cpp +++ b/src/platform/freertos/memory.cpp @@ -31,14 +31,16 @@ static constexpr size_t POOL_SIZE = SOMEIP_FREERTOS_MESSAGE_POOL_SIZE; -alignas(someip::Message) static char +namespace { + +alignas(someip::Message) char pool_buffer[POOL_SIZE * sizeof(someip::Message)]; -static bool block_used[POOL_SIZE] = {}; -static SemaphoreHandle_t pool_mutex = nullptr; -static std::atomic pool_initialized{false}; +bool block_used[POOL_SIZE] = {}; +SemaphoreHandle_t pool_mutex = nullptr; +std::atomic pool_initialized{false}; -static void ensure_pool_init() { +void ensure_pool_init() { if (pool_initialized.load(std::memory_order_acquire)) { return; } @@ -57,6 +59,26 @@ static void ensure_pool_init() { taskEXIT_CRITICAL(); } +void release_message_impl(someip::Message* msg) { + if (!msg) { + return; + } + + msg->~Message(); + + auto* raw = reinterpret_cast(msg); + size_t const offset = static_cast(raw - pool_buffer); + size_t const index = offset / sizeof(someip::Message); + + if (index < POOL_SIZE) { + xSemaphoreTake(pool_mutex, portMAX_DELAY); + block_used[index] = false; + xSemaphoreGive(pool_mutex); + } +} + +} // namespace + namespace someip::platform { /** @implements REQ_PLATFORM_FREERTOS_002 */ @@ -83,21 +105,7 @@ MessagePtr allocate_message() { } void release_message(Message* msg) { - if (!msg) { - return; - } - - msg->~Message(); - - auto* raw = reinterpret_cast(msg); - size_t const offset = static_cast(raw - pool_buffer); - size_t const index = offset / sizeof(Message); - - if (index < POOL_SIZE) { - xSemaphoreTake(pool_mutex, portMAX_DELAY); - block_used[index] = false; - xSemaphoreGive(pool_mutex); - } + release_message_impl(msg); } } // namespace someip::platform diff --git a/src/platform/threadx/memory.cpp b/src/platform/threadx/memory.cpp index c82fc42c5f..5dd9eb5ce1 100644 --- a/src/platform/threadx/memory.cpp +++ b/src/platform/threadx/memory.cpp @@ -30,14 +30,17 @@ static constexpr size_t POOL_SIZE = SOMEIP_THREADX_MESSAGE_POOL_SIZE; -alignas(someip::Message) static UCHAR - pool_buffer[POOL_SIZE * sizeof(someip::Message)]; - TX_BLOCK_POOL message_pool; -static TX_MUTEX pool_guard; std::atomic pool_initialized{false}; -static void ensure_pool_init() { +namespace { + +alignas(someip::Message) UCHAR + pool_buffer[POOL_SIZE * sizeof(someip::Message)]; + +TX_MUTEX pool_guard; + +void ensure_pool_init() { if (pool_initialized.load(std::memory_order_acquire)) { return; } @@ -67,6 +70,17 @@ static void ensure_pool_init() { TX_RESTORE } +void release_message_impl(someip::Message* msg) { + if (!msg) { + return; + } + + msg->~Message(); + tx_block_release(static_cast(msg)); +} + +} // namespace + namespace someip::platform { /** @implements REQ_PLATFORM_THREADX_002 */ @@ -86,12 +100,7 @@ MessagePtr allocate_message() { } void release_message(Message* msg) { - if (!msg) { - return; - } - - msg->~Message(); - tx_block_release(static_cast(msg)); + release_message_impl(msg); } } // namespace someip::platform diff --git a/src/rpc/rpc_client.cpp b/src/rpc/rpc_client.cpp index 868d79d89a..c188670cf6 100644 --- a/src/rpc/rpc_client.cpp +++ b/src/rpc/rpc_client.cpp @@ -162,7 +162,7 @@ class RpcClientImpl : public transport::ITransportListener { PendingCall call_info{ service_id, method_id, session_id, std::chrono::steady_clock::now(), - timeout, callback + timeout, std::move(callback) }; RpcCallHandle handle = 0; @@ -305,7 +305,7 @@ RpcCallHandle RpcClient::call_method_async(uint16_t service_id, MethodId method_ const std::vector& parameters, RpcCallback callback, const RpcTimeout& timeout) { - return impl_->call_method_async(service_id, method_id, parameters, callback, timeout); + return impl_->call_method_async(service_id, method_id, parameters, std::move(callback), timeout); } bool RpcClient::cancel_call(RpcCallHandle handle) { diff --git a/src/rpc/rpc_server.cpp b/src/rpc/rpc_server.cpp index fe6e778193..d99507213c 100644 --- a/src/rpc/rpc_server.cpp +++ b/src/rpc/rpc_server.cpp @@ -20,6 +20,7 @@ #include "common/result.h" #include #include +#include namespace someip::rpc { @@ -79,7 +80,7 @@ class RpcServerImpl : public transport::ITransportListener { // Check if already registered bool already_exists = method_handlers_.count(method_id) > 0; if (!already_exists) { - method_handlers_[method_id] = handler; + method_handlers_[method_id] = std::move(handler); } return !already_exists; } @@ -160,7 +161,7 @@ class RpcServerImpl : public transport::ITransportListener { } /** @implements REQ_MSG_115, REQ_MSG_117, REQ_MSG_117_E01 */ - void send_success_response(MessagePtr request, const transport::Endpoint& sender, + void send_success_response(MessagePtr const& request, const transport::Endpoint& sender, const std::vector& return_values) { MessageId response_msg_id(request->get_service_id(), request->get_method_id()); Message response(response_msg_id, request->get_request_id(), @@ -174,7 +175,7 @@ class RpcServerImpl : public transport::ITransportListener { } /** @implements REQ_MSG_115, REQ_MSG_117, REQ_MSG_117_E01, REQ_MSG_129 */ - void send_error_response(MessagePtr request, const transport::Endpoint& sender, ReturnCode error_code) { + void send_error_response(MessagePtr const& request, const transport::Endpoint& sender, ReturnCode error_code) { MessageId response_msg_id(request->get_service_id(), request->get_method_id()); Message response(response_msg_id, request->get_request_id(), MessageType::ERROR, error_code); @@ -227,7 +228,7 @@ void RpcServer::shutdown() { } bool RpcServer::register_method(MethodId method_id, MethodHandler handler) { - return impl_->register_method(method_id, handler); + return impl_->register_method(method_id, std::move(handler)); } bool RpcServer::unregister_method(MethodId method_id) { diff --git a/src/sd/sd_client.cpp b/src/sd/sd_client.cpp index 447f1c69b1..fafe6ff6c6 100644 --- a/src/sd/sd_client.cpp +++ b/src/sd/sd_client.cpp @@ -24,7 +24,9 @@ namespace someip::sd { -static std::shared_ptr create_sd_transport(const SdConfig& config) { +namespace { + +std::shared_ptr create_sd_transport(const SdConfig& config) { transport::UdpTransportConfig cfg; cfg.reuse_port = true; cfg.multicast_interface = config.unicast_address; @@ -32,6 +34,8 @@ static std::shared_ptr create_sd_transport(const SdConf transport::Endpoint("0.0.0.0", config.multicast_port), cfg); } +} // namespace + /** * @brief Service Discovery Client implementation * @implements REQ_ARCH_001 @@ -349,7 +353,7 @@ class SdClientImpl : public transport::ITransportListener { for (uint8_t i = 0; i < run1 && (index1 + i) < options.size(); ++i) { const auto& option = options[index1 + i]; if (option->get_type() == OptionType::IPV4_ENDPOINT) { - auto* ep = static_cast(option.get()); + const auto* ep = static_cast(option.get()); instance.ip_address = ep->get_ipv4_address_string(); instance.port = ep->get_port(); instance.protocol = ep->get_protocol(); diff --git a/src/sd/sd_message.cpp b/src/sd/sd_message.cpp index c3616b92b2..cac8bf569f 100644 --- a/src/sd/sd_message.cpp +++ b/src/sd/sd_message.cpp @@ -262,7 +262,7 @@ bool IPv4EndpointOption::deserialize(const std::vector& data, size_t& o << ((ipv4_address_ >> 24) & 0xFF) << "." << ((ipv4_address_ >> 16) & 0xFF) << "." << ((ipv4_address_ >> 8) & 0xFF) << "." - << (ipv4_address_ & 0xFF) << std::endl; + << (ipv4_address_ & 0xFF) << '\n'; // Continue processing despite invalid address } @@ -343,7 +343,7 @@ bool IPv4MulticastOption::deserialize(const std::vector& data, size_t& << ((ipv4_address_ >> 24) & 0xFF) << "." << ((ipv4_address_ >> 16) & 0xFF) << "." << ((ipv4_address_ >> 8) & 0xFF) << "." - << (ipv4_address_ & 0xFF) << std::endl; + << (ipv4_address_ & 0xFF) << '\n'; // Continue processing despite invalid address } port_ = (data[offset] << 8) | data[offset + 1]; diff --git a/src/sd/sd_server.cpp b/src/sd/sd_server.cpp index 10f5cea5dd..67ed9bb483 100644 --- a/src/sd/sd_server.cpp +++ b/src/sd/sd_server.cpp @@ -26,7 +26,9 @@ namespace someip::sd { -static std::shared_ptr create_sd_transport(const SdConfig& config) { +namespace { + +std::shared_ptr create_sd_transport(const SdConfig& config) { transport::UdpTransportConfig cfg; cfg.reuse_port = true; cfg.multicast_interface = config.unicast_address; @@ -34,6 +36,8 @@ static std::shared_ptr create_sd_transport(const SdConf transport::Endpoint("0.0.0.0", config.multicast_port), cfg); } +} // namespace + /** * @brief Service Discovery Server implementation * @implements REQ_ARCH_001 @@ -490,7 +494,7 @@ class SdServerImpl : public transport::ITransportListener { if (index1 < options.size()) { const auto& option = options[index1]; if (option->get_type() == OptionType::IPV4_ENDPOINT) { - auto* ep = static_cast(option.get()); + const auto* ep = static_cast(option.get()); client_ip = ep->get_ipv4_address_string(); client_port = ep->get_port(); client_protocol = ep->get_protocol(); diff --git a/src/someip/message.cpp b/src/someip/message.cpp index b9107036cb..11944a2b87 100644 --- a/src/someip/message.cpp +++ b/src/someip/message.cpp @@ -81,7 +81,7 @@ Message::Message(const Message& other) Message::Message(Message&& other) noexcept : message_id_(other.message_id_), - length_(8 + (other.e2e_header_.has_value() ? other.e2e_header_->get_header_size() : 0) + other.payload_.size()), + length_(8 + (other.e2e_header_.has_value() ? e2e::E2EHeader::get_header_size() : 0) + other.payload_.size()), request_id_(other.request_id_), protocol_version_(other.protocol_version_), interface_version_(other.interface_version_), @@ -115,7 +115,7 @@ Message& Message::operator=(const Message& other) { Message& Message::operator=(Message&& other) noexcept { if (this != &other) { message_id_ = other.message_id_; - length_ = 8 + (other.e2e_header_.has_value() ? other.e2e_header_->get_header_size() : 0) + other.payload_.size(); + length_ = 8 + (other.e2e_header_.has_value() ? e2e::E2EHeader::get_header_size() : 0) + other.payload_.size(); request_id_ = other.request_id_; protocol_version_ = other.protocol_version_; interface_version_ = SOMEIP_INTERFACE_VERSION; // Valid interface version for moved-to object @@ -266,7 +266,7 @@ bool Message::deserialize(const std::vector& data, bool expect_e2e) { if (length_ < 8) { return false; // Invalid length: must be at least 8 for header } - size_t e2e_size = e2e_header_.has_value() ? e2e_header_->get_header_size() : 0; + size_t e2e_size = e2e_header_.has_value() ? e2e::E2EHeader::get_header_size() : 0; size_t const expected_payload_size = length_ - 8 - e2e_size; size_t actual_payload_size = data.size() - offset; @@ -313,14 +313,10 @@ bool Message::has_valid_method_id() const { uint16_t const method_id = get_method_id(); // REQ_MSG_008: Reserved Method ID 0xFFFF is invalid - if (method_id == 0xFFFF) { - return false; - } - // REQ_MSG_006: Method IDs for methods (0x0001-0x7FFF) are valid // REQ_MSG_007: Method IDs for events (0x8001-0x8FFF) are valid // Allow 0x0000 for default/uninitialized messages - return true; // Allow all valid method IDs including 0x0000 + return method_id != 0xFFFF; } /** @@ -338,14 +334,9 @@ bool Message::has_valid_message_id() const { */ bool Message::has_valid_length() const { // REQ_MSG_012: Minimum length value - if (length_ < 8) { - return false; - } - // REQ_MSG_015: Length must be at least minimum header size // Additional validation happens in has_valid_header() - - return true; + return length_ >= 8; } /** @@ -464,7 +455,7 @@ bool Message::has_valid_header() const { // Check length consistency // length_ contains length from client_id to end (8 + e2e_size + payload_size) // Total expected message size should be HEADER_SIZE + e2e_size + payload_size - size_t e2e_size = e2e_header_.has_value() ? e2e_header_->get_header_size() : 0; + size_t e2e_size = e2e_header_.has_value() ? e2e::E2EHeader::get_header_size() : 0; uint32_t expected_total_size = HEADER_SIZE + e2e_size + payload_.size(); uint32_t actual_total_size = HEADER_SIZE + e2e_size + payload_.size(); // Same calculation if (expected_total_size != actual_total_size) { @@ -529,7 +520,7 @@ void Message::update_length() { // SOME/IP length field contains length from client_id to end of message // client_id(2) + session_id(2) + protocol_version(1) + interface_version(1) + // message_type(1) + return_code(1) + e2e_header_size + payload_size - size_t e2e_size = e2e_header_.has_value() ? e2e_header_->get_header_size() : 0; + size_t e2e_size = e2e_header_.has_value() ? e2e::E2EHeader::get_header_size() : 0; length_ = 8 + e2e_size + payload_.size(); } diff --git a/src/tp/tp_reassembler.cpp b/src/tp/tp_reassembler.cpp index da59945813..c4662ae147 100644 --- a/src/tp/tp_reassembler.cpp +++ b/src/tp/tp_reassembler.cpp @@ -59,7 +59,7 @@ bool TpReassembler::parse_tp_header(const std::vector& payload, // Check offset alignment (REQ_TP_015_E01) if (offset % 16 != 0) { // Log warning but continue processing - std::cout << "Warning: Received TP segment with misaligned offset: " << offset << std::endl; + std::cout << "Warning: Received TP segment with misaligned offset: " << offset << '\n'; } // Extract more segments flag (bit 0) diff --git a/src/transport/tcp_transport.cpp b/src/transport/tcp_transport.cpp index 6ac97a5a75..46c6ad175e 100644 --- a/src/transport/tcp_transport.cpp +++ b/src/transport/tcp_transport.cpp @@ -56,7 +56,7 @@ Result TcpTransport::initialize(const Endpoint& local_endpoint) { // Update local endpoint with the actual bound port (useful when port was 0) sockaddr_in bound_addr = {}; socklen_t addr_len = sizeof(bound_addr); - if (someip_getsockname(connection_.socket_fd, (sockaddr*)&bound_addr, &addr_len) == 0) { + if (someip_getsockname(connection_.socket_fd, reinterpret_cast(&bound_addr), &addr_len) == 0) { local_endpoint_ = Endpoint(local_endpoint_.get_address(), ntohs(bound_addr.sin_port)); } @@ -211,7 +211,8 @@ someip_socket_t TcpTransport::accept_connection() { sockaddr_in client_addr = {}; socklen_t client_len = sizeof(client_addr); - someip_socket_t client_fd = someip_accept(listen_socket_fd_, (sockaddr*)&client_addr, &client_len); + someip_socket_t client_fd = + someip_accept(listen_socket_fd_, reinterpret_cast(&client_addr), &client_len); if (client_fd == SOMEIP_INVALID_SOCKET) { return SOMEIP_INVALID_SOCKET; @@ -244,7 +245,7 @@ Result TcpTransport::bind_socket() { addr.sin_port = htons(local_endpoint_.get_port()); addr.sin_addr.s_addr = someip_inet_addr(local_endpoint_.get_address().c_str()); - if (someip_bind(connection_.socket_fd, (sockaddr*)&addr, sizeof(addr)) < 0) { + if (someip_bind(connection_.socket_fd, reinterpret_cast(&addr), sizeof(addr)) < 0) { return Result::NETWORK_ERROR; } @@ -300,7 +301,8 @@ Result TcpTransport::connect_internal(const Endpoint& endpoint) { connection_.state = TcpConnectionState::CONNECTING; connection_.remote_endpoint = endpoint; - int connect_result = someip_connect(connection_.socket_fd, (sockaddr*)&addr, sizeof(addr)); + int connect_result = + someip_connect(connection_.socket_fd, reinterpret_cast(&addr), sizeof(addr)); if (connect_result == 0) { // Connected immediately diff --git a/tests/test_message.cpp b/tests/test_message.cpp index 709d52d83e..24a4968494 100644 --- a/tests/test_message.cpp +++ b/tests/test_message.cpp @@ -300,6 +300,60 @@ TEST_F(MessageTest, LengthValidation) { EXPECT_FALSE(msg.has_valid_length()); } +/** + * @test_case TC_MSG_003_BOUNDARY + * @tests REQ_MSG_003 + * @brief Boundary tests for method ID validation + */ +TEST_F(MessageTest, MethodIdValidationBoundary) { + Message msg; + + msg.set_method_id(0x0000); + EXPECT_TRUE(msg.has_valid_method_id()); + + msg.set_method_id(0x0001); + EXPECT_TRUE(msg.has_valid_method_id()); + + msg.set_method_id(0xFFFE); + EXPECT_TRUE(msg.has_valid_method_id()); + + msg.set_method_id(0xFFFF); + EXPECT_FALSE(msg.has_valid_method_id()); +} + +/** + * @test_case TC_MSG_012_BOUNDARY + * @tests REQ_MSG_012 + * @brief Boundary tests for length validation + */ +TEST_F(MessageTest, LengthValidationBoundary) { + Message msg; + + msg.set_length(0); + EXPECT_FALSE(msg.has_valid_length()); + + msg.set_length(7); + EXPECT_FALSE(msg.has_valid_length()); + + msg.set_length(8); + EXPECT_TRUE(msg.has_valid_length()); + + msg.set_length(9); + EXPECT_TRUE(msg.has_valid_length()); + + msg.set_length(UINT32_MAX); + EXPECT_TRUE(msg.has_valid_length()); +} + +/** + * @test_case TC_MSG_STATIC_ACCESS + * @brief Verify E2E header size is accessible via static method + */ +TEST_F(MessageTest, E2EHeaderSizeStaticAccess) { + constexpr size_t header_size = e2e::E2EHeader::get_header_size(); + EXPECT_EQ(header_size, 12u); +} + /** * @test_case TC_MSG_021 * @tests REQ_MSG_021, REQ_MSG_022 diff --git a/tests/test_tp.cpp b/tests/test_tp.cpp index a5506d2ebc..83c4386bcf 100644 --- a/tests/test_tp.cpp +++ b/tests/test_tp.cpp @@ -683,3 +683,19 @@ TEST_F(TpTest, MessageExceedsMaxSize) { tp_manager.shutdown(); } + +/** + * @test_case TC_TP_TRANSFER_INIT + * @brief Verify TpTransfer initializes start_time and last_activity consistently + */ +TEST(TpTypesTest, TransferInitTimestamps) { + auto before = std::chrono::steady_clock::now(); + someip::tp::TpTransfer transfer(42, 1400); + auto after = std::chrono::steady_clock::now(); + + EXPECT_EQ(transfer.transfer_id, 42u); + EXPECT_EQ(transfer.total_size, 1400u); + EXPECT_GE(transfer.start_time, before); + EXPECT_LE(transfer.start_time, after); + EXPECT_EQ(transfer.start_time, transfer.last_activity); +} From 05c208b5632f871d14e0c91696f4e479e02c18a3 Mon Sep 17 00:00:00 2001 From: Vinicius Zein Date: Mon, 13 Apr 2026 03:50:52 -0400 Subject: [PATCH 2/3] fix(test): correct TpTransfer test to use message_id, not total_size Made-with: Cursor --- tests/test_tp.cpp | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/tests/test_tp.cpp b/tests/test_tp.cpp index 83c4386bcf..5f123204d6 100644 --- a/tests/test_tp.cpp +++ b/tests/test_tp.cpp @@ -690,11 +690,11 @@ TEST_F(TpTest, MessageExceedsMaxSize) { */ TEST(TpTypesTest, TransferInitTimestamps) { auto before = std::chrono::steady_clock::now(); - someip::tp::TpTransfer transfer(42, 1400); + someip::tp::TpTransfer transfer(42, 0x12345678); auto after = std::chrono::steady_clock::now(); EXPECT_EQ(transfer.transfer_id, 42u); - EXPECT_EQ(transfer.total_size, 1400u); + EXPECT_EQ(transfer.message_id, 0x12345678u); EXPECT_GE(transfer.start_time, before); EXPECT_LE(transfer.start_time, after); EXPECT_EQ(transfer.start_time, transfer.last_activity); From a59d487857084d54458c3d57a88cc37a9822e55f Mon Sep 17 00:00:00 2001 From: Vinicius Zein Date: Mon, 13 Apr 2026 09:20:28 -0400 Subject: [PATCH 3/3] fix: address CodeRabbit review findings - Validate FreeRTOS pool ownership before destroying Message (bounds + alignment check prevents UB on foreign pointers) - Replace CRC32 mutable globals with thread-safe static local initialization (C++ magic statics guarantee thread safety) - Move SubscriptionInfo into map instead of copying - Fix move assignment to preserve interface_version_ from source - Remove no-op identical comparison in has_valid_header - Add move assignment test for interface_version preservation Made-with: Cursor --- src/e2e/e2e_crc.cpp | 40 ++++++++++++++------------------ src/events/event_subscriber.cpp | 2 +- src/platform/freertos/memory.cpp | 23 +++++++++++------- src/someip/message.cpp | 11 +-------- tests/test_message.cpp | 18 ++++++++++++++ 5 files changed, 51 insertions(+), 43 deletions(-) diff --git a/src/e2e/e2e_crc.cpp b/src/e2e/e2e_crc.cpp index eadaeb571e..fe60b33c30 100644 --- a/src/e2e/e2e_crc.cpp +++ b/src/e2e/e2e_crc.cpp @@ -13,6 +13,7 @@ #include "e2e/e2e_crc.h" #include +#include #include #include @@ -76,36 +77,29 @@ static constexpr uint32_t CRC32_INIT = 0xFFFFFFFF; namespace { -// Precomputed CRC32 lookup table -uint32_t crc32_table[256]; - -// Initialize CRC32 lookup table (called once) -bool crc32_table_initialized = false; - -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; +const std::array& get_crc32_table() { + static const std::array table = [] { + std::array t{}; + 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; + } } + t[i] = crc; } - crc32_table[i] = crc; - } - - crc32_table_initialized = true; + return t; + }(); + return table; } } // namespace uint32_t calculate_crc32(const std::vector& data) { - init_crc32_table(); + const auto& crc32_table = get_crc32_table(); uint32_t crc = CRC32_INIT; diff --git a/src/events/event_subscriber.cpp b/src/events/event_subscriber.cpp index a09ff47d3b..38daff7252 100644 --- a/src/events/event_subscriber.cpp +++ b/src/events/event_subscriber.cpp @@ -98,7 +98,7 @@ class EventSubscriberImpl : public transport::ITransportListener { // Store subscription platform::ScopedLock const subs_lock(subscriptions_mutex_); std::string key = make_subscription_key(service_id, instance_id, eventgroup_id); - subscriptions_[key] = sub_info; + subscriptions_[key] = std::move(sub_info); // Send subscription request via RPC (simplified - in real implementation, // this would use SD to find the service endpoint and send subscription) diff --git a/src/platform/freertos/memory.cpp b/src/platform/freertos/memory.cpp index 31f3c34fb7..1dd56f44a8 100644 --- a/src/platform/freertos/memory.cpp +++ b/src/platform/freertos/memory.cpp @@ -64,17 +64,22 @@ void release_message_impl(someip::Message* msg) { return; } - msg->~Message(); - auto* raw = reinterpret_cast(msg); - size_t const offset = static_cast(raw - pool_buffer); - size_t const index = offset / sizeof(someip::Message); - - if (index < POOL_SIZE) { - xSemaphoreTake(pool_mutex, portMAX_DELAY); - block_used[index] = false; - xSemaphoreGive(pool_mutex); + auto* pool_start = static_cast(pool_buffer); + if (raw < pool_start || raw >= pool_start + POOL_SIZE * sizeof(someip::Message)) { + return; + } + size_t const byte_offset = static_cast(raw - pool_start); + if (byte_offset % sizeof(someip::Message) != 0) { + return; } + size_t const index = byte_offset / sizeof(someip::Message); + + msg->~Message(); + + xSemaphoreTake(pool_mutex, portMAX_DELAY); + block_used[index] = false; + xSemaphoreGive(pool_mutex); } } // namespace diff --git a/src/someip/message.cpp b/src/someip/message.cpp index 11944a2b87..6a21a343ab 100644 --- a/src/someip/message.cpp +++ b/src/someip/message.cpp @@ -118,7 +118,7 @@ Message& Message::operator=(Message&& other) noexcept { length_ = 8 + (other.e2e_header_.has_value() ? e2e::E2EHeader::get_header_size() : 0) + other.payload_.size(); request_id_ = other.request_id_; protocol_version_ = other.protocol_version_; - interface_version_ = SOMEIP_INTERFACE_VERSION; // Valid interface version for moved-to object + interface_version_ = other.interface_version_; message_type_ = other.message_type_; return_code_ = other.return_code_; payload_ = std::move(other.payload_); // Move the payload @@ -453,16 +453,7 @@ bool Message::has_valid_header() const { } // Check length consistency - // length_ contains length from client_id to end (8 + e2e_size + payload_size) - // Total expected message size should be HEADER_SIZE + e2e_size + payload_size size_t e2e_size = e2e_header_.has_value() ? e2e::E2EHeader::get_header_size() : 0; - uint32_t expected_total_size = HEADER_SIZE + e2e_size + payload_.size(); - uint32_t actual_total_size = HEADER_SIZE + e2e_size + payload_.size(); // Same calculation - if (expected_total_size != actual_total_size) { - return false; - } - - // Also check that length_ is consistent uint32_t expected_length = 8 + e2e_size + payload_.size(); if (length_ != expected_length) { return false; diff --git a/tests/test_message.cpp b/tests/test_message.cpp index 24a4968494..92a0344af3 100644 --- a/tests/test_message.cpp +++ b/tests/test_message.cpp @@ -411,6 +411,24 @@ TEST_F(MessageTest, CopyAndMove) { EXPECT_FALSE(original.is_valid()); } +/** + * @test_case TC_MSG_MOVE_ASSIGN + * @brief Move assignment preserves interface_version from source + */ +TEST_F(MessageTest, MoveAssignmentPreservesInterfaceVersion) { + Message source; + source.set_service_id(0x1234); + source.set_interface_version(0x42); + source.set_payload({0xAA, 0xBB}); + + Message target; + target = std::move(source); + + EXPECT_EQ(target.get_service_id(), 0x1234); + EXPECT_EQ(target.get_interface_version(), 0x42); + EXPECT_EQ(target.get_payload(), (std::vector{0xAA, 0xBB})); +} + /** * @test_case TC_MSG_006 * @tests REQ_MSG_051, REQ_MSG_052, REQ_MSG_053, REQ_MSG_054