From 77ea540fddcd38ea9d662c3e4b7c5b0e3ac2b648 Mon Sep 17 00:00:00 2001 From: Marc Durdin Date: Wed, 22 Oct 2025 11:27:35 +0200 Subject: [PATCH] refactor(core): DRY out filling of sub-section data and fix cross-platform diffs * Address cross-platform compiler warnings around shadowed pointers and non-virtual destructors. * Refactor the offset calculations for sub-section data to reduce repetition both within each calculation, and the patterns of the calculations themselves. This mostly resolves possibility of typo errors when mapping the section data in, e.g. using the wrong count, as each variable and type is only referenced once, and compiler will catch most discrepancies - except for count vs type. This is also much easier to read and verify in code review, I hope! --- core/src/kmx/kmx_plus.cpp | 377 +++++++++++++++++++------------------- core/src/kmx/kmx_plus.h | 13 +- 2 files changed, 196 insertions(+), 194 deletions(-) diff --git a/core/src/kmx/kmx_plus.cpp b/core/src/kmx/kmx_plus.cpp index 234e91c145..013697608a 100644 --- a/core/src/kmx/kmx_plus.cpp +++ b/core/src/kmx/kmx_plus.cpp @@ -341,26 +341,94 @@ const U* get_file_data_at_offset(const COMP_KMXPLUS_SECT *base, COMP_KMXPLUS_HEA /** * @brief Get the section data at offset, casting to desired type * - * @tparam T - * @tparam U - * @param base start of the section data, i.e. immedaitely after section header + * @tparam T struct type of the section + * @tparam U struct type of data to return + * @param base start of the section data, i.e. immediately after section + * header * @param header header data for the section * @param offset offset in bytes from the start of the section data * @param size size of the data to return, for validation - * @return U + * @return pointer to data, type U */ template -U get_section_data_at_offset(const T *base, COMP_KMXPLUS_HEADER const &header, KMX_DWORD offset, KMX_DWORD size) { +const U* get_section_data_at_offset(const T *base, COMP_KMXPLUS_HEADER const &header, KMX_DWORD offset, KMX_DWORD size) { if(!is_block_valid(header, offset, size)) { return nullptr; } offset -= header.headerSize(); const uint8_t* thisptr = reinterpret_cast(base); - U start = reinterpret_cast(thisptr+offset); + const U* start = reinterpret_cast(thisptr+offset); return start; } +/** + * @brief Get the section data at offset, casting to desired type, verify that + * the section is long enough for count * data, and update the offset to point + * to the next byte after the data. Allows zero-length, optional data. + * + * @tparam T struct type of the section + * @tparam U struct type of data to return + * @param base start of the section data, i.e. immediately after section + * header + * @param header header data for the section + * @param count number of U items expected + * @param offset (in, out) offset in bytes from the start of the section data, + * updated on return to next byte after data + * @param out (out) pointer to start of data + * @return bool false on error + */ +template +bool get_optional_section_data_at_offset_and_increment(const T *base, COMP_KMXPLUS_HEADER const &header, + KMX_DWORD count, KMX_DWORD& offset, const U*& out +) { + out = nullptr; + + if(count == 0) { + return true; + } + + KMX_DWORD size = count * sizeof(U); + + out = get_section_data_at_offset(base, header, offset, size); + + if(out == nullptr) { + return false; + } + + offset += size; + + return true; +} + +/** + * @brief Get the section data at offset, casting to desired type, verify that + * the section is long enough for count * data, and update the offset to point + * to the next byte after the data. Does not allow zero-length, optional data. + * + * @tparam T struct type of the section + * @tparam U struct type of data to return + * @param base start of the section data, i.e. immediately after section + * header + * @param header header data for the section + * @param count number of U items expected + * @param offset (in, out) offset in bytes from the start of the section data, + * updated on return to next byte after data + * @param out (out) pointer to start of data + * @return bool false on error or missing data + */ +template +bool get_required_section_data_at_offset_and_increment(const T *base, COMP_KMXPLUS_HEADER const &header, + KMX_DWORD count, KMX_DWORD& offset, const U*& out +) { + if(count == 0) { + out = nullptr; + return false; + } + + return get_optional_section_data_at_offset_and_increment(base, header, count, offset, out); +} + bool COMP_KMXPLUS_HEADER_17::valid(KMX_DWORD length) const { DebugLog("%c%c%c%c: (%X) size 0x%X\n", DEBUG_IDENT(ident), ident, size); @@ -429,7 +497,7 @@ COMP_KMXPLUS_META::valid(COMP_KMXPLUS_HEADER const &header, KMX_DWORD _kmn_unuse bool COMP_KMXPLUS_DISP::valid(COMP_KMXPLUS_HEADER const &header, KMX_DWORD _kmn_unused(length)) const { DebugLog("disp: count 0x%X\n", count); - if (header.size < sizeof(*this)+(sizeof(entries[0])*count)) { + if (!is_block_valid(header, header.headerSize(), sizeof(*this)+(sizeof(entries[0])*count))) { DebugLog("header.size < expected size"); assert(false); return false; @@ -459,7 +527,7 @@ COMP_KMXPLUS_STRS::valid(COMP_KMXPLUS_HEADER const &header, KMX_DWORD _kmn_unuse for (KMX_DWORD i=0; i(this, header, offset, length); + const KMX_WCHAR* start = get_section_data_at_offset(this, header, offset, (length+1)*sizeof(KMX_WCHAR)); if(!start) { return false; } @@ -655,7 +723,7 @@ COMP_KMXPLUS_ELEM::getElementList(const COMP_KMXPLUS_HEADER &header, KMX_DWORD e } // pointer to specified entry - return get_section_data_at_offset(this, header, entry.offset, + return get_section_data_at_offset(this, header, entry.offset, entry.length * sizeof(COMP_KMXPLUS_ELEM_ELEMENT)); } @@ -739,6 +807,11 @@ COMP_KMXPLUS_TRAN_Helper::getReorder(KMX_DWORD reorder) const { bool COMP_KMXPLUS_TRAN_Helper::set(const COMP_KMXPLUS_TRAN *newTran) { + is_valid = false; + groups = nullptr; + transforms = nullptr; + reorders = nullptr; + if(!COMP_KMXPLUS_Section_Helper::set(newTran)) { return false; } @@ -752,44 +825,20 @@ COMP_KMXPLUS_TRAN_Helper::set(const COMP_KMXPLUS_TRAN *newTran) { } KMX_DWORD offset = this->header.calculateBaseSize(LDML_LENGTH_TRAN); - // groups - if (data()->groupCount > 0) { - groups = get_section_data_at_offset(data(), header, offset, - sizeof(COMP_KMXPLUS_TRAN_GROUP) * data()->groupCount); - } else { - groups = nullptr; - } + // groups (required) + is_valid = is_valid && get_required_section_data_at_offset_and_increment( + data(), header, data()->groupCount, offset, groups); + assert(is_valid); - if(!groups) { - is_valid = false; - assert(is_valid); - } - offset += sizeof(COMP_KMXPLUS_TRAN_GROUP) * data()->groupCount; + // transforms (optional) + is_valid = is_valid && get_optional_section_data_at_offset_and_increment( + data(), header, data()->transformCount, offset, transforms); + assert(is_valid); - // transforms - if (data()->transformCount > 0) { - transforms = get_section_data_at_offset(data(), header, offset, - sizeof(COMP_KMXPLUS_TRAN_TRANSFORM) * data()->transformCount); - if(!transforms) { - is_valid = false; - assert(is_valid); - } - } else { - transforms = nullptr; - } - offset += sizeof(COMP_KMXPLUS_TRAN_TRANSFORM) * data()->transformCount; - - // reorders - if (data()->reorderCount > 0) { - reorders = get_section_data_at_offset(data(), header, offset, - sizeof(COMP_KMXPLUS_TRAN_REORDER) * data()->reorderCount); - if(!reorders) { - is_valid = false; - assert(is_valid); - } - } else { - reorders = nullptr; - } + // reorders (optional) + is_valid = is_valid && get_optional_section_data_at_offset_and_increment( + data(), header, data()->reorderCount, offset, reorders); + assert(is_valid); // Now, validate offsets by walking if (is_valid) { @@ -873,7 +922,15 @@ COMP_KMXPLUS_LAYR_Helper::COMP_KMXPLUS_LAYR_Helper() : is_valid(false) { bool COMP_KMXPLUS_LAYR_Helper::set(const COMP_KMXPLUS_LAYR *newLayr) { - COMP_KMXPLUS_Section_Helper::set(newLayr); + is_valid = false; + lists = nullptr; + entries = nullptr; + rows = nullptr; + keys = nullptr; + + if(!COMP_KMXPLUS_Section_Helper::set(newLayr)) { + return false; + } DebugLog("validating newLayr=%p", newLayr); is_valid = true; if (newLayr == nullptr) { @@ -882,53 +939,26 @@ COMP_KMXPLUS_LAYR_Helper::set(const COMP_KMXPLUS_LAYR *newLayr) { return true; // not invalid, just missing } KMX_DWORD offset = this->header.calculateBaseSize(LDML_LENGTH_LAYR); // skip past non-dynamic portion - // lists - if (data()->listCount > 0) { - lists = get_section_data_at_offset(data(), header, offset, - sizeof(COMP_KMXPLUS_LAYR_LIST) * data()->listCount); - } else { - lists = nullptr; - } - if(!lists) { - is_valid = false; - assert(is_valid); - } - offset += sizeof(COMP_KMXPLUS_LAYR_LIST) * data()->listCount; - // entries - if (data()->layerCount > 0) { - entries = get_section_data_at_offset(data(), header, offset, - sizeof(COMP_KMXPLUS_LAYR_ENTRY) * data()->layerCount); - } else { - entries = nullptr; - } - if(!entries) { - is_valid = false; - assert(is_valid); - } - offset += sizeof(COMP_KMXPLUS_LAYR_ENTRY) * data()->layerCount; - // rows - if (data()->rowCount > 0) { - rows = get_section_data_at_offset(data(), header, offset, - sizeof(COMP_KMXPLUS_LAYR_ROW) * data()->rowCount); - } else { - rows = nullptr; - } - if(!rows) { - is_valid = false; - assert(is_valid); - } - offset += sizeof(COMP_KMXPLUS_LAYR_ROW) * data()->rowCount; - // keys - if (data()->keyCount > 0) { - keys = get_section_data_at_offset(data(), header, offset, - sizeof(COMP_KMXPLUS_LAYR_KEY) * data()->keyCount); - } else { - keys = nullptr; - } - if(!keys) { - is_valid = false; - assert(is_valid); - } + + // lists (required) + is_valid = is_valid && get_required_section_data_at_offset_and_increment( + data(), header, data()->listCount, offset, lists); + assert(is_valid); + + // entries (required) - note, "entryCount" is called "layerCount" in COMP_KMXPLUS_LAYR + is_valid = is_valid && get_required_section_data_at_offset_and_increment( + data(), header, data()->layerCount, offset, entries); + assert(is_valid); + + // rows (required) + is_valid = is_valid && get_required_section_data_at_offset_and_increment( + data(), header, data()->rowCount, offset, rows); + assert(is_valid); + + // keys (required) + is_valid = is_valid && get_required_section_data_at_offset_and_increment( + data(), header, data()->keyCount, offset, keys); + assert(is_valid); // Now, validate offsets by walking if (is_valid) { @@ -1039,7 +1069,15 @@ COMP_KMXPLUS_KEYS_Helper::COMP_KMXPLUS_KEYS_Helper() : is_valid(false) { bool COMP_KMXPLUS_KEYS_Helper::set(const COMP_KMXPLUS_KEYS *newKeys) { - COMP_KMXPLUS_Section_Helper::set(newKeys); + is_valid = false; + keys = nullptr; + flickLists = nullptr; + flickElements = nullptr; + kmap = nullptr; + + if(!COMP_KMXPLUS_Section_Helper::set(newKeys)) { + return false; + } DebugLog("validating newKeys=%p", newKeys); is_valid = true; if (newKeys == nullptr) { @@ -1047,54 +1085,28 @@ COMP_KMXPLUS_KEYS_Helper::set(const COMP_KMXPLUS_KEYS *newKeys) { // which validates this section's length. Will be nullptr here if invalid. return true; // not invalid, just missing } - // keys + KMX_DWORD offset = this->header.calculateBaseSize(LDML_LENGTH_KEYS); - if (data()->keyCount > 0) { - keys = get_section_data_at_offset(data(), header, offset, - sizeof(COMP_KMXPLUS_KEYS_KEY) * data()->keyCount); - } else { - keys = nullptr; - } - if(!keys) { - is_valid = false; - assert(is_valid); - } - offset += sizeof(COMP_KMXPLUS_KEYS_KEY) * data()->keyCount; - // flicks - if (data()->flicksCount > 0) { - flickLists = get_section_data_at_offset(data(), header, offset, - sizeof(COMP_KMXPLUS_KEYS_FLICK_LIST) * data()->flicksCount); - if(!flickLists) { - is_valid = false; - assert(is_valid); - } - } else { - flickLists = nullptr; // not an error - } - offset += sizeof(COMP_KMXPLUS_KEYS_FLICK_LIST) * data()->flicksCount; - // flick - if (data()->flickCount > 0) { - flickElements = get_section_data_at_offset(data(), header, offset, - sizeof(COMP_KMXPLUS_KEYS_FLICK_ELEMENT) * data()->flickCount); - if(!flickElements) { - is_valid = false; - assert(is_valid); - } - } else { - flickElements = nullptr; // not an error - } - offset += sizeof(COMP_KMXPLUS_KEYS_FLICK_ELEMENT) * data()->flickCount; - // kmap - if (data()->kmapCount > 0) { - kmap = get_section_data_at_offset(data(), header, offset, - sizeof(COMP_KMXPLUS_KEYS_KMAP) * data()->kmapCount); - if(!kmap) { - is_valid = false; - assert(is_valid); - } - } else { - kmap = nullptr; // not an error - } + + // keys (required) + is_valid = is_valid && get_required_section_data_at_offset_and_increment( + data(), header, data()->keyCount, offset, keys); + assert(is_valid); + + // flicks (optional) - note tricky "flicksCount" vs "flickCount" in COMP_KMXPLUS_KEYS + is_valid = is_valid && get_optional_section_data_at_offset_and_increment( + data(), header, data()->flicksCount, offset, flickLists); + assert(is_valid); + + // flick (optional) - note tricky "flicksCount" vs "flickCount" in COMP_KMXPLUS_KEYS + is_valid = is_valid && get_optional_section_data_at_offset_and_increment( + data(), header, data()->flickCount, offset, flickElements); + assert(is_valid); + + // kmap (optional) + is_valid = is_valid && get_optional_section_data_at_offset_and_increment( + data(), header, data()->kmapCount, offset, kmap); + assert(is_valid); // Now, validate offsets by walking if (is_valid) { @@ -1253,7 +1265,13 @@ COMP_KMXPLUS_LIST_Helper::COMP_KMXPLUS_LIST_Helper() : is_valid(false) { bool COMP_KMXPLUS_LIST_Helper::set(const COMP_KMXPLUS_LIST *newList) { - COMP_KMXPLUS_Section_Helper::set(newList); + is_valid = false; + lists = nullptr; + indices = nullptr; + + if(!COMP_KMXPLUS_Section_Helper::set(newList)) { + return false; + } DebugLog("validating newList=%p", newList); is_valid = true; if (newList == nullptr) { @@ -1263,28 +1281,17 @@ COMP_KMXPLUS_LIST_Helper::set(const COMP_KMXPLUS_LIST *newList) { } KMX_DWORD offset = this->header.calculateBaseSize(LDML_LENGTH_LIST); // skip past non-dynamic portion - // lists - if (data()->listCount > 0) { - lists = get_section_data_at_offset(data(), header, offset, sizeof(COMP_KMXPLUS_LIST_ITEM) * data()->listCount); - if(!lists) { - is_valid = false; - assert(is_valid); - } - } else { - lists = nullptr; - // not invalid, just empty. - } - offset += sizeof(COMP_KMXPLUS_LIST_ITEM) * data()->listCount; - // entries - if (data()->indexCount > 0) { - indices = get_section_data_at_offset(data(), header, offset, sizeof(COMP_KMXPLUS_LIST_INDEX) * data()->indexCount); - if(!indices) { - is_valid = false; - assert(is_valid); - } - } else { - indices = nullptr; - } + + // lists (optional) + is_valid = is_valid && get_optional_section_data_at_offset_and_increment( + data(), header, data()->listCount, offset, lists); + assert(is_valid); + + // indices (optional) + is_valid = is_valid && get_optional_section_data_at_offset_and_increment( + data(), header, data()->indexCount, offset, indices); + assert(is_valid); + // Now, validate offsets by walking if (is_valid) { for (KMX_DWORD i = 0; is_valid && i < data()->listCount; i++) { @@ -1358,7 +1365,13 @@ COMP_KMXPLUS_USET_Helper::COMP_KMXPLUS_USET_Helper() : is_valid(false), usets(nu bool COMP_KMXPLUS_USET_Helper::set(const COMP_KMXPLUS_USET *newUset) { - COMP_KMXPLUS_Section_Helper::set(newUset); + is_valid = false; + usets = nullptr; + ranges = nullptr; + + if(!COMP_KMXPLUS_Section_Helper::set(newUset)) { + return false; + } DebugLoad("validating newUset=%p", newUset); is_valid = true; if (newUset == nullptr) { @@ -1367,28 +1380,16 @@ COMP_KMXPLUS_USET_Helper::set(const COMP_KMXPLUS_USET *newUset) { return true; // not invalid, just missing } KMX_DWORD offset = this->header.calculateBaseSize(LDML_LENGTH_USET); // skip past non-dynamic portion - // usets - if (data()->usetCount > 0) { - usets = get_section_data_at_offset(data(), header, offset, sizeof(COMP_KMXPLUS_USET_USET) * data()->usetCount); - if(!usets) { - is_valid = false; - assert(is_valid); - } - } else { - usets = nullptr; - // not invalid, just empty. - } - offset += sizeof(COMP_KMXPLUS_USET_USET) * data()->usetCount; - // entries - if (data()->rangeCount > 0) { - ranges = get_section_data_at_offset(data(), header, offset, sizeof(COMP_KMXPLUS_USET_RANGE) * data()->rangeCount); - if(!ranges) { - is_valid = false; - assert(is_valid); - } - } else { - ranges = nullptr; - } + + // usets (optional) + is_valid = is_valid && get_optional_section_data_at_offset_and_increment( + data(), header, data()->usetCount, offset, usets); + assert(is_valid); + + // ranges (optional) + is_valid = is_valid && get_optional_section_data_at_offset_and_increment( + data(), header, data()->rangeCount, offset, ranges); + assert(is_valid); // Now, validate offsets by walking // is_valid must be true at this point. @@ -1581,7 +1582,7 @@ COMP_KMXPLUS_STRS::get(const COMP_KMXPLUS_HEADER& header, KMX_DWORD entry) const const KMX_DWORD length = entries[entry].length; // the string is null terminated in the data file, thus length + 1 - auto start = get_section_data_at_offset(this, header, offset, (length + 1) * sizeof(KMX_WCHAR)); + auto start = get_section_data_at_offset(this, header, offset, (length + 1) * sizeof(KMX_WCHAR)); if(!start) { return std::u16string(); } diff --git a/core/src/kmx/kmx_plus.h b/core/src/kmx/kmx_plus.h index 0d4344709b..d6eff001d2 100644 --- a/core/src/kmx/kmx_plus.h +++ b/core/src/kmx/kmx_plus.h @@ -101,16 +101,16 @@ public: * @param size * @return KMX_DWORD */ - inline KMX_DWORD calculateBaseSize(KMX_DWORD size) { - return size - LDML_LENGTH_HEADER_17 + this->_headerSize; + inline KMX_DWORD calculateBaseSize(KMX_DWORD elementSize) { + return elementSize - LDML_LENGTH_HEADER_17 + this->_headerSize; } - inline void set(KMX_DWORD fileVersion, KMXPLUS_IDENT ident, KMX_DWORD size, KMX_DWORD version = LDML_KMXPLUS_VERSION_17) { + inline void set(KMX_DWORD fileVersion, KMXPLUS_IDENT identIn, KMX_DWORD sizeIn, KMX_DWORD versionIn = LDML_KMXPLUS_VERSION_17) { this->_fileVersion = fileVersion; this->_headerSize = fileVersion == LDML_KMXPLUS_VERSION_17 ? LDML_LENGTH_HEADER_17 : LDML_LENGTH_HEADER_19; - this->ident = ident; - this->size = size; - this->version = version; + this->ident = identIn; + this->size = sizeIn; + this->version = versionIn; } }; @@ -136,6 +136,7 @@ protected: public: COMP_KMXPLUS_HEADER header; COMP_KMXPLUS_Section_Helper() : _data(nullptr) {} + virtual ~COMP_KMXPLUS_Section_Helper() {} virtual bool set(const T* section) { _data = section; return true;