Fix static analysis violations across the Thunder core directory#2140
Draft
workkavint-ship-it wants to merge 14 commits into
Draft
Fix static analysis violations across the Thunder core directory#2140workkavint-ship-it wants to merge 14 commits into
workkavint-ship-it wants to merge 14 commits into
Conversation
Used delete[] instead of delete for array allocated with new[], which was causing heap corruption on buffer release. Also fixed two logical-AND (&&) operators that should have been bitwise-AND (&) when extracting the high byte of the length field, which was silently zeroing out the upper byte for any length value > 0xFF.
…e condition Added a null guard on the MapViewOfFile return value before passing it to UpdateCache a failed mapping would have written null into the buffer. Fixed the inverted INVALID_HANDLE_VALUE check in Reallocation so that MapViewOfFileEx is called with a valid handle, not a broken one.
Changed readlink result type from size_t to ssize_t and added a check so a failure (-1) no longer wraps to SIZE_MAX and writes out of bounds. Wrapped the pollfd array writes in a != nullptr check after malloc to prevent a null dereference when the system is out of memory.
…dows The handle was overwritten with INVALID_HANDLE_VALUE before the null check, making FindClose permanently unreachable. Moved the assignment after the close so every FindFirstFile handle gets properly released.
&MACAddress was copying the address of the local pointer variable instead of the MAC bytes it points to, producing garbage destination addresses in every Ethernet frame. Matches how SourceMAC already works.
When moving a Netlink object, the constructor was wiping its own freshly copied values instead of clearing the object it moved from. The new object always ended up empty. Now it correctly leaves the source in a reset state.
…ste bug State() was referencing m_Responded which doesn't exist — the actual member is m_State. Response() was returning *m_Request instead of *m_Response, a copy-paste mistake that gave callers the request back when they asked for the response.
Both MACAddress overloads on Windows were dereferencing the info pointer without checking if it was null first. On macOS, the interface list from getifaddrs was never freed causing a memory leak on every call, and the iteration was starting at ifa_next which silently dropped the first adapter.
If calloc failed, the code was doing pointer arithmetic on null and assigning it to the receive buffer undefined behavior and a crash. Wrapped both buffer assignments in a null check so they only run when the allocation actually succeeded.
On Apple builds m_pageSize was hardcoded to 0 while Linux used sysconf. Any call to GetPhysicalPageCount() on macOS would divide by zero and crash. Now uses sysconf(_SC_PAGESIZE) the same way Linux already does.
…er end _modules.find() was compared against _callsigns.end() instead of _modules.end(). Iterators from different containers are never equal so every module was permanently treated as excluded, silently disabling all warning reporting regardless of the exclusion list contents.
Size() was returning m_Offset - m_Buffer.Size() instead of the other way around. Since both are unsigned, when offset is less than the buffer size the subtraction wraps to a huge garbage value. IsValid() right above confirms the correct intent: remaining bytes = buffer size - offset.
…name strncpy with IFNAMSIZ-1 leaves the buffer unterminated when the interface name is exactly 15 characters long. Added explicit null termination at IFNAMSIZ-1 after both the Apple and Linux strncpy calls to guarantee the string is always safely terminated before being passed to the kernel.
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
We ran a structured static analysis pass on Thunder core components using a prompt-driven workflow and worked through the reported findings one issue at a time. For each finding, the issue was reviewed, the likely root cause was analyzed, and a proposed fix was evaluated before making any code changes. The goal of this PR is to address the reported violations while preserving the existing behavior of the affected components.
The findings covered a range of issues across 13 files, including potential null pointer dereferences, memory and handle management problems, incorrect condition checks, arithmetic underflow, move semantics issues, buffer handling edge cases, and platform-specific initialization concerns.