refactor: replace C-style array with std::array for buffer management

[dudantas]

Description

This pull request refactors the buffer management from using C-style arrays to std::array<uint8_t, NETWORKMESSAGE_MAXSIZE> in the NetworkMessage and OutputMessage classes. The modification enhances memory safety, ensures compile-time size checks, and improves code clarity by leveraging modern C++ best practices. Transitioning to std::array offers fixed-size buffer management with the benefits of type safety and better integration with the Standard Template Library (STL).

Additionally, this update includes several other improvements:

1. Removal of memcpy in Favor of std::ranges::copy: All instances of memcpy have been replaced with std::ranges::copy, providing a safer and more expressive way to copy data between containers while maintaining type safety and avoiding manual pointer arithmetic.

2. Replacement of memset with std::fill: The usage of memset for buffer initialization and padding has been replaced with std::fill, ensuring a more readable and type-safe approach to filling buffers with specific values.

3. Elimination of reinterpret_cast: Instances of reinterpret_cast used to convert data types have been removed. For example, string extraction from the buffer now uses direct string construction:

  std::string result(buffer.begin() + info.position, buffer.begin() + info.position + stringLen);

4. Introduce the use of std::source_location: The addString and getString functions, along with an optional function parameter. These enhancements improve logging by providing more contextual information about where and how the addString function is invoked, whether from C++ code or Lua scripts. This dual-parameter approach ensures flexibility and more precise debugging information.

5. Fixed bug in the "decodeHeader" function and added unit tests

Behaviour

Actual

• Memory management is handled using C-style arrays, which lack bounds checking and can lead to buffer overflows and undefined behavior if not managed correctly.
• Manual management of array indices increases the risk of memory-related bugs and complicates the codebase.
• Logging within addString relies solely on std::source_location, limiting contextual information when invoked from Lua scripts.

Expected

• Buffer management is handled using std::array<uint8_t, NETWORKMESSAGE_MAXSIZE>, which provides compile-time size checks and prevents buffer overflows through safer access methods.
• The use of std::array simplifies the code by eliminating the need for manual index management and reducing the risk of memory leaks and undefined behavior.
• The addString function continues to utilize std::source_location and an optional function parameter to enhance logging, providing detailed contextual information whether called from C++ or Lua scripts.
• Improved logging facilitates easier debugging by indicating the exact location and context of addString invocations.

Possible fixes this crash:

stack_smashing.log

Edit:

I confirmed that this fixed the crash, the problem was that a uint32_t was being added to a getString, which potentially caused an overflow, I was able to find out due to a log that was given and I saw that the header changed to something from the store: cfae88a

Type of change

• Bug fix (non-breaking change which fixes an issue)

How Has This Been Tested

The changes were tested by executing the following scenarios to ensure that buffer management and logging behaviors operate as expected:

• Manual Testing of Network Messages: Verified correct encoding and decoding of data using std::array<uint8_t, NETWORKMESSAGE_MAXSIZE>.
• Stress Testing with Large Messages: Validated buffer boundaries and memory safety under high-load conditions.
• Logging Verification: Ensured that addString logs correctly capture contextual information using both std::source_location and the optional function parameter.
• Integration Testing with Lua Scripts: Confirmed that Lua scripts can invoke addString with and without the function parameter, and that logs reflect the appropriate context.
• Automated Unit Tests: Updated and executed unit tests to cover new functionalities and ensure all tests pass successfully.

Checklist

• My code follows the style guidelines of this project
• I have performed a self-review of my own code
• I checked the PR checks reports
• I have commented my code, particularly in hard-to-understand areas
• I have made corresponding changes to the documentation
• My changes generate no new warnings
• I have added tests that prove my fix is effective or that my feature works

Benefits of Switching to std::array from C-Style Arrays

1. Compile-Time Size Checking

std::array provides compile-time size information, ensuring that buffer sizes are consistent and reducing the likelihood of buffer overflows that are common with C-style arrays.

std::array<uint8_t, NETWORKMESSAGE_MAXSIZE> buffer = {0};

2. Improved Memory Safety

• Bounds Checking: While C-style arrays do not perform bounds checking, std::array offers member functions like at() that throw exceptions on out-of-bounds access, enhancing memory safety.
• No Decay to Pointers: std::array does not decay to pointers implicitly, preventing unintended pointer arithmetic and related bugs.

3. Better Integration with STL Algorithms

std::array seamlessly integrates with STL algorithms, allowing for more expressive and concise code. Functions like std::copy, std::fill, and range-based for loops work naturally with std::array.

std::copy(value.begin(), value.end(), buffer.begin() + info.position);

4. Enhanced Readability and Maintainability

Using std::array makes the intention of fixed-size buffer management explicit, improving code readability and maintainability. It clearly communicates that the buffer size is fixed and known at compile time.

5. No Dynamic Memory Allocation Overhead

std::array` provides a fixed-size buffer at compile-time, ensuring that no dynamic memory allocation or reallocation occurs during its lifetime.

[github-actions]

Qodana for C/C++

1020 new problems were found

Inspection name	Severity	Problems
misra-cpp2008-5-0-11	🔶 Warning	890
err33-c	🔶 Warning	37
static-accessed-through-instance	🔶 Warning	16
misra-cpp2008-5-0-5	🔶 Warning	13
misra-cpp2008-5-2-12	🔶 Warning	5
function-cognitive-complexity	🔶 Warning	5
misra-cpp2008-4-5-2	🔶 Warning	5
magic-numbers	🔶 Warning	4
narrowing-conversions	🔶 Warning	3
unnecessary-value-param	🔶 Warning	3
misra-cpp2008-0-1-7	🔶 Warning	3
implicit-bool-conversion	🔶 Warning	3
use-starts-ends-with	🔶 Warning	3
signed-bitwise	🔶 Warning	3
misra-cpp2008-5-3-1	🔶 Warning	2
misra-cpp2008-5-0-13	🔶 Warning	2
unroll-loops	🔶 Warning	2
convert-member-functions-to-static	🔶 Warning	2
unnecessary-copy-initialization	🔶 Warning	2
id-dependent-backward-branch	🔶 Warning	2
duplicate-include	🔶 Warning	2
simplify-boolean-expr	🔶 Warning	2
misra-cpp2008-6-4-5	🔶 Warning	1
no-automatic-move	🔶 Warning	1
misra-cpp2008-6-5-3	🔶 Warning	1
pro-bounds-constant-array-index	🔶 Warning	1
else-after-return	🔶 Warning	1
misra-cpp2008-5-8-1	🔶 Warning	1
too-small-loop-variable	🔶 Warning	1
pass-by-value	🔶 Warning	1
inconsistent-declaration-parameter-name	🔶 Warning	1
for-range-copy	🔶 Warning	1
redundant-casting	🔶 Warning	1

💡 Qodana analysis was run in the pull request mode: only the changed files were checked

View the detailed Qodana report

To be able to view the detailed Qodana report, you can either:

• Register at Qodana Cloud and configure the action
• Use GitHub Code Scanning with Qodana
• Host Qodana report at GitHub Pages
• Inspect and use qodana.sarif.json (see the Qodana SARIF format for details)

To get *.log files or any other Qodana artifacts, run the action with upload-result option set to true,
so that the action will upload the files as the job artifacts:

      - name: 'Qodana Scan'
        uses: JetBrains/[email protected]
        with:
          upload-result: true

Contact Qodana team

Contact us at [email protected]

• Or via our issue tracker: https://jb.gg/qodana-issue
• Or share your feedback: https://jb.gg/qodana-discussions