From 130ad64597584ce88f34d6507d9de702147f22f8 Mon Sep 17 00:00:00 2001 From: Eduardo Dantas Date: Fri, 27 Sep 2024 08:24:38 -0300 Subject: [PATCH] fix: some qodana reviews and code improvements --- src/server/network/message/networkmessage.cpp | 77 ++++++++++++++----- src/server/network/message/networkmessage.hpp | 2 +- src/server/network/message/outputmessage.hpp | 13 +--- tests/build_and_run.sh | 2 +- 4 files changed, 64 insertions(+), 30 deletions(-) diff --git a/src/server/network/message/networkmessage.cpp b/src/server/network/message/networkmessage.cpp index bde962572c9..a7835438340 100644 --- a/src/server/network/message/networkmessage.cpp +++ b/src/server/network/message/networkmessage.cpp @@ -22,38 +22,75 @@ int32_t NetworkMessage::decodeHeader() { // Log the current position and buffer content before decoding g_logger().debug("[{}] Decoding header at position: {}", __FUNCTION__, info.position); g_logger().debug("[{}] Buffer content: ", __FUNCTION__); - for (size_t i = 0; i < 10; ++i) { - g_logger().debug("[{}] Buffer content: {}", __FUNCTION__, buffer[i]); + + // Log only up to 10 bytes of the buffer or until the end of the buffer + for (size_t i = 0; i < 10 && i < buffer.size(); ++i) { + g_logger().debug("[{}] Buffer[{}]: {}", __FUNCTION__, i, buffer[i]); } - if (info.position + sizeof(uint16_t) <= buffer.size()) { - // Creating a std::array with the bytes - std::array headerBytes = { buffer[info.position], buffer[info.position + 1] }; - uint16_t header = std::bit_cast(headerBytes); - info.position += sizeof(header); // Update position + // Check if there are enough bytes in the buffer for the header + if (info.position + 1 < buffer.size()) { + // Create a span view for safety + std::span bufferSpan(buffer.data(), buffer.size()); + + // Extract header bytes safely using std::bit_cast + uint16_t header = static_cast(bufferSpan[info.position]) | + (static_cast(bufferSpan[info.position + 1]) << 8); + + // Update position after reading the header + info.position += sizeof(header); + + // Return the decoded header return header; + } else { + g_logger().warn("Index out of bounds when trying to access buffer with position: {}", info.position); } // Handle buffer overflow error here - g_logger().error("[NetworkMessage::decodeHeader] - Attempted to read beyond buffer limits at position: {}", info.position); - return 0; + g_logger().error("[{}] attempted to read beyond buffer limits at position: {}", __FUNCTION__, info.position); + return {}; } // Simply read functions for incoming message uint8_t NetworkMessage::getByte() { + // Check if there is at least 1 byte to read if (!canRead(1)) { - return 0; + g_logger().error("[{}] Not enough data to read a byte. Current position: {}, Length: {}", __FUNCTION__, info.position, info.length); + return {}; + } + + // Ensure that position is within bounds before decrementing + if (info.position == 0) { + g_logger().error("[{}] Position is at the beginning of the buffer. Cannot decrement.", __FUNCTION__); + return {}; } - return buffer[info.position++]; + try { + // Decrement position safely and return the byte + return buffer.at(--info.position); + } catch (const std::out_of_range& e) { + g_logger().error("[{}] Out of range error: {}. Position: {}, Buffer size: {}", __FUNCTION__, e.what(), info.position, buffer.size()); + } + + return {}; } uint8_t NetworkMessage::getPreviousByte() { + // Check if position is at the beginning of the buffer if (info.position == 0) { - g_logger().error("Attempted to get previous byte at position 0"); - return 0; + g_logger().error("[{}] Attempted to get previous byte at position 0", __FUNCTION__); + return {}; // Return default value (0) when at the start of the buffer } - return buffer[--info.position]; + + try { + // Safely decrement position and access the previous byte using 'at()' + return buffer.at(--info.position); + } catch (const std::out_of_range& e) { + // Log the out-of-range exception if accessing outside buffer limits + g_logger().error("[{}] Out of range error: {}. Position: {}, Buffer size: {}", __FUNCTION__, e.what(), info.position, buffer.size()); + } + + return {}; } std::string NetworkMessage::getString(uint16_t stringLen /* = 0*/, const std::source_location &location) { @@ -128,7 +165,7 @@ void NetworkMessage::addString(const std::string &value, const std::source_locat g_logger().trace("[{}] called line '{}:{}' in '{}'", __FUNCTION__, location.line(), location.column(), location.function_name()); } - uint16_t len = static_cast(stringLen); + auto len = static_cast(stringLen); add(len); // Using to copy the string into the buffer auto it = std::ranges::copy(value, buffer.begin() + info.position); @@ -146,9 +183,9 @@ double NetworkMessage::getDouble() { // Retrieve the precision byte from the buffer uint8_t precision = getByte(); // Retrieve the scaled uint32_t value from the buffer - uint32_t scaledValue = get(); + auto scaledValue = get(); // Convert the scaled value back to double using the precision factor - double adjustedValue = static_cast(scaledValue) - std::numeric_limits::max(); + double adjustedValue = static_cast(scaledValue) - static_cast(std::numeric_limits::max()); // Convert back to the original double value using the precision factor return adjustedValue / std::pow(static_cast(SCALING_BASE), precision); } @@ -239,14 +276,16 @@ const uint8_t* NetworkMessage::getBuffer() const { uint8_t* NetworkMessage::getBodyBuffer() { info.position = 2; - return buffer.data() + HEADER_LENGTH; + // Return the pointer to the body of the buffer starting after the header + // Convert HEADER_LENGTH to uintptr_t to ensure safe pointer arithmetic with enum type + return buffer.data() + static_cast(HEADER_LENGTH); } bool NetworkMessage::canAdd(size_t size) const { return (size + info.position) < MAX_BODY_LENGTH; } -bool NetworkMessage::canRead(int32_t size) { +bool NetworkMessage::canRead(int32_t size) const { return size <= (info.length - (info.position - INITIAL_BUFFER_POSITION)); } diff --git a/src/server/network/message/networkmessage.hpp b/src/server/network/message/networkmessage.hpp index f890c086037..1889b7725b6 100644 --- a/src/server/network/message/networkmessage.hpp +++ b/src/server/network/message/networkmessage.hpp @@ -151,7 +151,7 @@ class NetworkMessage { bool canAdd(size_t size) const; - bool canRead(int32_t size); + bool canRead(int32_t size) const; void append(const NetworkMessage &other); diff --git a/src/server/network/message/outputmessage.hpp b/src/server/network/message/outputmessage.hpp index 3197c476057..459e243cea3 100644 --- a/src/server/network/message/outputmessage.hpp +++ b/src/server/network/message/outputmessage.hpp @@ -41,11 +41,8 @@ class OutputMessage : public NetworkMessage { void append(const NetworkMessage &msg) { auto msgLen = msg.getLength(); - // Create a span for the source data std::span sourceSpan(msg.getBuffer() + INITIAL_BUFFER_POSITION, msgLen); - // Create a span for the destination in the buffer std::span destSpan(buffer.data() + info.position, msgLen); - // Copy the data using std::ranges::copy std::ranges::copy(sourceSpan, destSpan.begin()); info.length += msgLen; info.position += msgLen; @@ -53,11 +50,8 @@ class OutputMessage : public NetworkMessage { void append(const OutputMessage_ptr &msg) { auto msgLen = msg->getLength(); - // Create a span for the source data std::span sourceSpan(msg->getBuffer() + INITIAL_BUFFER_POSITION, msgLen); - // Create a span for the destination in the buffer std::span destSpan(buffer.data() + info.position, msgLen); - // Copy the data using std::ranges::copy std::ranges::copy(sourceSpan, destSpan.begin()); info.length += msgLen; info.position += msgLen; @@ -75,13 +69,14 @@ class OutputMessage : public NetworkMessage { assert(outputBufferStart >= sizeof(T)); // Move the buffer position back to make space for the header outputBufferStart -= sizeof(T); - // Ensure that T is trivially copyable + static_assert(std::is_trivially_copyable_v, "Type T must be trivially copyable"); + // Convert the header to an array of unsigned char using std::bit_cast auto byteArray = std::bit_cast>(addHeader); - // Create a span from the byte array + std::span byteSpan(byteArray); - // Copy the bytes into the buffer using std::copy + // Copy the bytes into the buffer std::ranges::copy(byteSpan, buffer.begin() + outputBufferStart); // Update the message size info.length += sizeof(T); diff --git a/tests/build_and_run.sh b/tests/build_and_run.sh index 5d8a2c2e423..ac4ccad512c 100755 --- a/tests/build_and_run.sh +++ b/tests/build_and_run.sh @@ -7,7 +7,7 @@ if [ ! -d "build" ]; then mkdir build fi cd build || exit -cmake -DCMAKE_BUILD_TYPE=Debug -DPACKAGE_TESTS=On .. ; make -j$(nproc) +cmake -DCMAKE_BUILD_TYPE=Debug -DPACKAGE_TESTS=On .. ; make -j"$(nproc)" ./tests/unit/canary_ut --reporter compact --success cd .. cd tests || exit