fix(memorytracker): correct StringTable search and entry sizing#20
Open
Rakdos8 wants to merge 1 commit into
Open
fix(memorytracker): correct StringTable search and entry sizing#20Rakdos8 wants to merge 1 commit into
Rakdos8 wants to merge 1 commit into
Conversation
StringTable::Find walked a m_id-ascending table with its comparison branches inverted, so present ids were not found, duplicates were inserted and the table's sort order broke (LookUp returned garbage). Swap the branches and set ix to the insertion point on miss. AddString sized the StringEntry block with sizeof(TableEntry) (the wrong struct), omitting the StringEntry header, so the string was written past the reserved block and corrupted neighbouring entries in the fixed arena. Size from offsetof(StringEntry, m_string) and pass the matching available length to strcpy_s.
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
Fixes a guaranteed-wrong binary search and an arena buffer overflow in the
memory tracker's string table.
Problem
StringTable::Findwalked anm_id-ascending table with its comparisonbranches inverted: present ids were not found, duplicates were inserted,
sort order broke, and
LookUpreturned garbage.AddStringsized theStringEntryblock withsizeof(TableEntry)(thewrong struct), omitting the
StringEntryheader, so the string waswritten past the reserved block and corrupted neighbouring entries in
the fixed 512 KB arena.
Fix
Swap the search branches and set
ixto the insertion point on miss.Size the entry from
offsetof(StringEntry, m_string)and pass the matchingavailable length to
strcpy_s. Adds#include <cstddef>foroffsetof.Type
Correctness + memory corruption (Critical for a foundational lib).
Testing
Manual review; search now lower-bound correct, allocation accounts for the
full variable-length-struct header.