From 90be01385b03ca3044d7ef487ebe3949264912cd Mon Sep 17 00:00:00 2001 From: "Steven R. Loomis" Date: Wed, 31 Aug 2022 16:31:42 -0500 Subject: [PATCH 1/3] =?UTF-8?q?chore(core):=20split=20out=20validators=20?= =?UTF-8?q?=F0=9F=99=80?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit #7136 --- core/src/kmx/kmx_plus.cpp | 282 ++++++++++++++++++------------- core/src/kmx/kmx_plus.h | 148 +++++++--------- core/src/ldml/ldml_processor.cpp | 105 ++++++------ 3 files changed, 279 insertions(+), 256 deletions(-) diff --git a/core/src/kmx/kmx_plus.cpp b/core/src/kmx/kmx_plus.cpp index 59bedc994c..86fddc4460 100644 --- a/core/src/kmx/kmx_plus.cpp +++ b/core/src/kmx/kmx_plus.cpp @@ -9,6 +9,7 @@ #include #if KMXPLUS_DEBUG +#include #include #endif @@ -18,18 +19,21 @@ namespace km { namespace kbp { namespace kmx { -/** - * @brief Usage: `KMXPLUS_PRINTF(("str: %s\n", "something"));` - * Note double parens - * \def KMXPLUS_DEBUG - */ #if KMXPLUS_DEBUG +#define debug_println(x) std::cerr << __FILE__ << ":" << __LINE__ << ": " << (x) << std::endl; #define KMXPLUS_PRINTF(x) printf x #else +#define debug_println(x) #define KMXPLUS_PRINTF(x) #endif - +/** + * @brief Validate (and print out) a section name dword + * + * @param ident the hex dword + * @return true if valid + * @return false if invalid + */ static bool validate_section_name(KMX_DWORD ident) { bool valid = true; @@ -46,33 +50,98 @@ validate_section_name(KMX_DWORD ident) { return valid; } -static bool -validate_kmxplus_header(const COMP_KMXPLUS_HEADER* hdr) { - if (hdr == nullptr) { - KMXPLUS_PRINTF(("! dump_kmxplus_header: NULL header\n")); +/** + * @brief cast to a COMP_KMXPLUS_HEADER from bytes + * + * @param data + * @param length + * @param ident + * @return const kmx::COMP_KMXPLUS_HEADER* + */ +static const kmx::COMP_KMXPLUS_HEADER * +header_from_bytes(const uint8_t *data, KMX_DWORD length, uint32_t ident) { + if (!data) { + debug_println("!data"); + return nullptr; + } + if (length < LDML_LENGTH_HEADER) { + debug_println("length < LDML_LENGTH_HEADER"); + } + const COMP_KMXPLUS_HEADER *all = reinterpret_cast(data); + if (!all->valid(length)) { + debug_println("header failed validation"); + return nullptr; + } + if (all->ident != ident) { + debug_println("header had wrong section id"); + return nullptr; + } + return all; +} + +/** + * @brief Accessor for a section based on bytes + * + * @tparam T + * @param data + * @param length + * @return const T* + */ +template +const T *section_from_bytes(const uint8_t *data, KMX_DWORD length) { + const COMP_KMXPLUS_HEADER *header = header_from_bytes(data, length, T::IDENT); + const T *section = reinterpret_cast(header); + if (section != nullptr && section->valid(length)) { + return section; + } else { + return nullptr; + } +} + +template +const T *section_from_sect(const COMP_KMXPLUS_SECT* sect) { + const uint8_t *rawbytes = reinterpret_cast(sect); + if (rawbytes == nullptr) { + debug_println("section_from_sect(nullptr) == nullptr"); + return nullptr; + } + KMX_DWORD offset = sect->find(T::IDENT); + if (!offset) { + debug_println("section_from_sect() - not found"); + return nullptr; + } + KMX_DWORD entrylength = sect->total - offset; + return section_from_bytes(rawbytes+offset, entrylength); +} + +bool +COMP_KMXPLUS_HEADER::valid(KMX_DWORD length) const { + KMXPLUS_PRINTF((": (%X) size 0x%X\n", ident, size)); + if (size < LDML_LENGTH_HEADER) { + debug_println("size < LDML_LENGTH_HEADER"); return false; } - if(!validate_section_name(hdr->ident)) { - KMXPLUS_PRINTF((" (invalid section name) \n")); + if (size > length) { + debug_println("size > length"); + return false; + } + if (!validate_section_name(ident)) { + debug_println("invalid section name"); return false; } - KMXPLUS_PRINTF((": (%X) size 0x%X\n", hdr->ident, hdr->size)); return true; } -static bool -validate_kmxplus_keys(const uint8_t* /*data*/, const COMP_KMXPLUS_KEYS* keys) { - if(keys == nullptr) { - KMXPLUS_PRINTF(("! could not load 'keys' section\n")); +bool +COMP_KMXPLUS_KEYS::valid(KMX_DWORD _kmn_unused(length)) const { + KMXPLUS_PRINTF((" count: #0x%X\n", this->count)); + if (header.size < sizeof(*this)+(sizeof(entries[0])*count)) { + debug_println("header.size < expected size"); return false; } - if(!validate_kmxplus_header((const COMP_KMXPLUS_HEADER*)keys)) { - return false; - } - KMXPLUS_PRINTF((" count: #0x%X\n", keys->count)); - for (KMX_DWORD i = 0; icount; i++) { + for (KMX_DWORD i = 0; icount; i++) { KMXPLUS_PRINTF((" #0x%d\n", i)); - const COMP_KMXPLUS_KEYS_ENTRY& entry = keys->entries[i]; + const COMP_KMXPLUS_KEYS_ENTRY& entry = this->entries[i]; KMXPLUS_PRINTF((" vkey\t0x%X\n", entry.vkey)); KMXPLUS_PRINTF((" mod\t0x%X\n", entry.mod)); KMXPLUS_PRINTF((" flags\t0x%X\n", entry.flags)); @@ -85,48 +154,40 @@ validate_kmxplus_keys(const uint8_t* /*data*/, const COMP_KMXPLUS_KEYS* keys) { return true; } -static bool -validate_kmxplus_loca(const uint8_t* /*data*/, const COMP_KMXPLUS_LOCA* loca) { - if(loca == nullptr) { - KMXPLUS_PRINTF(("! could not load 'loca' section\n")); - return false; - } - if(!validate_kmxplus_header((const COMP_KMXPLUS_HEADER*)loca)) { +bool +COMP_KMXPLUS_LOCA::valid(KMX_DWORD _kmn_unused(length)) const { + if (header.size < sizeof(*this)+(sizeof(entries[0])*count)) { + debug_println("header.size < expected size"); return false; } // TODO-LDML - for(KMX_DWORD i=0; icount; i++) { - KMXPLUS_PRINTF((" Locale #%d: #0x%X\n", i, loca->entries[i].locale)); + for(KMX_DWORD i=0; icount; i++) { + KMXPLUS_PRINTF((" Locale #%d: #0x%X\n", i, this->entries[i].locale)); } return true; } -static bool -validate_kmxplus_meta(const uint8_t* /*data*/, const COMP_KMXPLUS_META* meta) { - if(meta == nullptr) { - KMXPLUS_PRINTF(("! could not load 'meta' section\n")); +bool +COMP_KMXPLUS_META::valid(KMX_DWORD _kmn_unused(length)) const { + if (header.size < sizeof(*this)) { + debug_println("header.size < expected size"); return false; } - if (!validate_kmxplus_header((const COMP_KMXPLUS_HEADER*)meta)) { - return false; - } - KMXPLUS_PRINTF((" name:\t#0x%X\n", meta->name)); - KMXPLUS_PRINTF((" author:\t#0x%X\n", meta->author)); - KMXPLUS_PRINTF((" conform:\t#0x%X\n", meta->conform)); - KMXPLUS_PRINTF((" layout:\t#0x%X\n", meta->layout)); - KMXPLUS_PRINTF((" normalization:\t#0x%X\n", meta->normalization)); - KMXPLUS_PRINTF((" indicator:\t#0x%X\n", meta->indicator)); - KMXPLUS_PRINTF((" settings:\t0x%X\n", meta->settings)); + KMXPLUS_PRINTF((" name:\t#0x%X\n", this->name)); + KMXPLUS_PRINTF((" author:\t#0x%X\n", this->author)); + KMXPLUS_PRINTF((" conform:\t#0x%X\n", this->conform)); + KMXPLUS_PRINTF((" layout:\t#0x%X\n", this->layout)); + KMXPLUS_PRINTF((" normalization:\t#0x%X\n", this->normalization)); + KMXPLUS_PRINTF((" indicator:\t#0x%X\n", this->indicator)); + KMXPLUS_PRINTF((" settings:\t0x%X\n", this->settings)); return true; } -static bool -validate_kmxplus_vkey(const uint8_t* /*data*/, const COMP_KMXPLUS_VKEY* vkey) { - if (vkey == nullptr) { - KMXPLUS_PRINTF(("! could not load 'vkey' section\n")); - return false; - } - if(!validate_kmxplus_header((const COMP_KMXPLUS_HEADER*)vkey)) { +bool +COMP_KMXPLUS_VKEY::valid(KMX_DWORD _kmn_unused(length)) const { + KMXPLUS_PRINTF(("vkey: count 0x%X\n", this->count)); + if (header.size < sizeof(*this)+(sizeof(entries[0])*count)) { + debug_println("header.size < expected size"); return false; } // TODO-LDML @@ -134,21 +195,18 @@ validate_kmxplus_vkey(const uint8_t* /*data*/, const COMP_KMXPLUS_VKEY* vkey) { return true; } -static bool -validate_kmxplus_strs(const uint8_t* /*data*/, const COMP_KMXPLUS_STRS* strs) { - if (strs == nullptr) { - KMXPLUS_PRINTF(("! could not load 'strs' section\n")); +bool +COMP_KMXPLUS_STRS::valid(KMX_DWORD _kmn_unused(length)) const { + KMXPLUS_PRINTF(("strs: count 0x%X\n", this->count)); + if (header.size < sizeof(*this)+(sizeof(entries[0])*count)) { + debug_println("header.size < expected size"); return false; } - if(!validate_kmxplus_header((const COMP_KMXPLUS_HEADER*)strs)) { - return false; - } - KMXPLUS_PRINTF(("strs: count 0x%X\n", strs->count)); - for (KMX_DWORD i=0; icount; i++) { + for (KMX_DWORD i=0; icount; i++) { const size_t MYBUFSIZ = 256; KMX_WCHAR buf[MYBUFSIZ]; KMXPLUS_PRINTF(("#0x%X: ", i)); - PKMX_WCHAR str = strs->get(i, buf, MYBUFSIZ); + PKMX_WCHAR str = this->get(i, buf, MYBUFSIZ); if (!str) { KMXPLUS_PRINTF(("NULL/ERR\n")); return false; @@ -165,83 +223,70 @@ validate_kmxplus_strs(const uint8_t* /*data*/, const COMP_KMXPLUS_STRS* strs) { return true; } -static bool -validate_kmxplus_sect(const uint8_t* data, const COMP_KMXPLUS_SECT* sect) { - if (sect == nullptr) { - KMXPLUS_PRINTF(("! could not load 'sect' section\n")); +bool +COMP_KMXPLUS_SECT::valid(KMX_DWORD length) const { + KMXPLUS_PRINTF(("sect: total 0x%X\n", this->total)); + KMXPLUS_PRINTF(("sect: count 0x%X\n", this->count)); + if (header.size < sizeof(*this)+(sizeof(entries[0])*count)) { + debug_println("header.size < expected size"); return false; } - if (!validate_kmxplus_header((const COMP_KMXPLUS_HEADER*)sect)) { - return false; - } - KMXPLUS_PRINTF(("sect: total 0x%X\n", sect->total)); - KMXPLUS_PRINTF(("sect: count 0x%X\n", sect->count)); + // now validate each component bool overall_valid = true; - - for (KMX_DWORD i = 0; i < sect->count; i++) { - const COMP_KMXPLUS_SECT_ENTRY& entry = sect->entries[i]; + for (KMX_DWORD i = 0; i < this->count; i++) { + const COMP_KMXPLUS_SECT_ENTRY& entry = this->entries[i]; if(!validate_section_name(entry.sect)) { KMXPLUS_PRINTF((" (invalid section name) \n")); return false; } KMXPLUS_PRINTF((" sect#%d: %X @ %X\n", i, entry.sect, entry.offset)); - const uint8_t* entrydata = (data+entry.offset); - bool validity = false; - switch(entry.sect) { - case LDML_SECTIONID_KEYS: - validity = validate_kmxplus_keys(data, as_kmxplus_keys(entrydata)); - break; - case LDML_SECTIONID_LOCA: - validity = validate_kmxplus_loca(data, as_kmxplus_loca(entrydata)); - break; - case LDML_SECTIONID_META: - validity = validate_kmxplus_meta(data, as_kmxplus_meta(entrydata)); - break; - case LDML_SECTIONID_SECT: - KMXPLUS_PRINTF(("! Cowardly refusing to dump invalid nested 'sect' section.\n")); - validity = false; - break; - case LDML_SECTIONID_STRS: - validity = validate_kmxplus_strs(data, as_kmxplus_strs(entrydata)); - break; - case LDML_SECTIONID_VKEY: - validity = validate_kmxplus_vkey(data, as_kmxplus_vkey(entrydata)); - break; - default: - KMXPLUS_PRINTF(("Unknown section %X", entry.sect)); - validity = false; - break; + if (entry.sect == LDML_SECTIONID_SECT) { + debug_println("Invalid nested 'sect'"); + overall_valid = false; + continue; } - if (!validity) { - overall_valid = false; // if any section is invalid + const uint8_t* data = reinterpret_cast(this); + const uint8_t* entrydata = data + entry.offset; + KMX_DWORD entrylength = length - entry.offset; + // just validate header + if(header_from_bytes(entrydata, entrylength, entry.sect) == nullptr) { + KMXPLUS_PRINTF(("Invalid header %X", entry.sect)); + overall_valid = false; + continue; } } return overall_valid; } -bool -validate_kmxplus_data(const uint8_t* data) { - const COMP_KMXPLUS_SECT* sect = as_kmxplus_sect(data); - if (sect == nullptr) { - KMXPLUS_PRINTF(("Err: 'sect' NULL from %p\n", data)); - return false; - } - return validate_kmxplus_sect(data, sect); -} +kmx_plus::kmx_plus(const COMP_KEYBOARD *keyboard) + : keys(nullptr), loca(nullptr), meta(nullptr), + sect(nullptr), strs(nullptr), vkey(nullptr), valid(false) { -bool -validate_kmxplus_data(kmx::PCOMP_KEYBOARD keyboard) { - KMXPLUS_PRINTF(("dump_kmxplus_data(): Got a PCOMP_KEYBOARD at %p\n", keyboard)); + KMXPLUS_PRINTF(("kmx_plus(): Got a COMP_KEYBOARD at %p\n", keyboard)); if (!(keyboard->dwFlags & KF_KMXPLUS)) { KMXPLUS_PRINTF(("Err: flags KF_KMXPLUS not set\n")); - return false; + valid = false; + return; } const COMP_KEYBOARD_EX* ex = reinterpret_cast(keyboard); - KMXPLUS_PRINTF(("KMXPlus offset 0x%X, KMXPlus size 0x%X\n", ex->kmxplus.dpKMXPlus, ex->kmxplus.dwKMXPlusSize)); + KMXPLUS_PRINTF(("kmx_plus(): KMXPlus offset 0x%X, KMXPlus size 0x%X\n", ex->kmxplus.dpKMXPlus, ex->kmxplus.dwKMXPlusSize)); const uint8_t* rawdata = reinterpret_cast(keyboard); - return validate_kmxplus_data(rawdata + ex->kmxplus.dpKMXPlus); + + sect = section_from_bytes(rawdata, ex->kmxplus.dwKMXPlusSize); + if (sect == nullptr) { + KMXPLUS_PRINTF(("kmx_plus(): 'sect' did not validate\n")); + valid = false; + } else { + valid = true; + // load other sections + keys = section_from_sect(sect); + loca = section_from_sect(sect); + meta = section_from_sect(sect); + strs = section_from_sect(sect); + vkey = section_from_sect(sect); + } } KMX_DWORD COMP_KMXPLUS_SECT::find(KMX_DWORD ident) const { @@ -259,6 +304,7 @@ COMP_KMXPLUS_STRS::get(KMX_DWORD entry, PKMX_WCHAR buf, KMX_DWORD bufsiz) const if (entry >= count) { return nullptr; } + // TODO-LDML: will be improved in https://github.com/keymanapp/keyman/issues/7134 KMX_DWORD offset = entries[entry].offset; KMX_DWORD length = entries[entry].length; assert(bufsiz > (length+1)); // assert bufsiz big enough diff --git a/core/src/kmx/kmx_plus.h b/core/src/kmx/kmx_plus.h index 33feac07c8..e9033df0ba 100644 --- a/core/src/kmx/kmx_plus.h +++ b/core/src/kmx/kmx_plus.h @@ -37,6 +37,7 @@ namespace kmx { struct COMP_KMXPLUS_HEADER { KMX_DWORD ident; // 0000 Section name KMX_DWORD size; // 0004 Section length + bool valid(KMX_DWORD length) const; }; // Assert that the length matches the declared length @@ -48,6 +49,7 @@ struct COMP_KMXPLUS_SECT_ENTRY { }; struct COMP_KMXPLUS_SECT { + static const KMX_DWORD IDENT = LDML_SECTIONID_SECT; COMP_KMXPLUS_HEADER header; KMX_DWORD total; // 0008 KMXPlus entire length KMX_DWORD count; // 000C number of section headers @@ -59,6 +61,11 @@ struct COMP_KMXPLUS_SECT { * @return KMX_DWORD offset from beginning of kmxplus */ KMX_DWORD find(KMX_DWORD ident) const; + /** + * @brief True if section is valid. + * Does not validate the entire file. + */ + bool valid(KMX_DWORD length) const; }; // Assert that the length matches the declared length @@ -71,6 +78,7 @@ struct COMP_KMXPLUS_STRS_ENTRY { }; struct COMP_KMXPLUS_STRS { + static const KMX_DWORD IDENT = LDML_SECTIONID_STRS; COMP_KMXPLUS_HEADER header; KMX_DWORD count; // 0008 count of str entries KMX_DWORD reserved; // 000C padding @@ -85,12 +93,17 @@ struct COMP_KMXPLUS_STRS { * @return nullptr or a pointer to the output buffer */ PKMX_WCHAR get(KMX_DWORD entry, PKMX_WCHAR buf, KMX_DWORD bufsiz) const; + /** + * @brief True if section is valid. + */ + bool valid(KMX_DWORD length) const; }; static_assert(sizeof(struct COMP_KMXPLUS_STRS) % 0x10 == 0, "Structs prior to entries[] should align to 128-bit boundary"); static_assert(sizeof(struct COMP_KMXPLUS_STRS) == LDML_LENGTH_STRS, "mismatched size of section strs"); struct COMP_KMXPLUS_META { + static const KMX_DWORD IDENT = LDML_SECTIONID_META; COMP_KMXPLUS_HEADER header; KMX_DWORD name; KMX_DWORD author; @@ -99,6 +112,10 @@ struct COMP_KMXPLUS_META { KMX_DWORD normalization; KMX_DWORD indicator; KMX_DWORD settings; + /** + * @brief True if section is valid. + */ + bool valid(KMX_DWORD length) const; }; static_assert(sizeof(struct COMP_KMXPLUS_META) == LDML_LENGTH_META, "mismatched size of section meta"); @@ -108,10 +125,15 @@ struct COMP_KMXPLUS_LOCA_ENTRY { }; struct COMP_KMXPLUS_LOCA { + static const KMX_DWORD IDENT = LDML_SECTIONID_LOCA; COMP_KMXPLUS_HEADER header; KMX_DWORD count; // 0008 number of locales KMX_DWORD reserved; COMP_KMXPLUS_LOCA_ENTRY entries[]; + /** + * @brief True if section is valid. + */ + bool valid(KMX_DWORD length) const; }; static_assert(sizeof(struct COMP_KMXPLUS_LOCA) % 0x10 == 0, "Structs prior to entries[] should align to 128-bit boundary"); @@ -125,10 +147,15 @@ struct COMP_KMXPLUS_KEYS_ENTRY { }; struct COMP_KMXPLUS_KEYS { + static const KMX_DWORD IDENT = LDML_SECTIONID_KEYS; COMP_KMXPLUS_HEADER header; KMX_DWORD count; // number of keys KMX_DWORD reserved; // padding COMP_KMXPLUS_KEYS_ENTRY entries[]; + /** + * @brief True if section is valid. + */ + bool valid(KMX_DWORD length) const; }; static_assert(sizeof(struct COMP_KMXPLUS_KEYS) % 0x10 == 0, "Structs prior to entries[] should align to 128-bit boundary"); @@ -140,111 +167,52 @@ struct COMP_KMXPLUS_VKEY_ENTRY { }; struct COMP_KMXPLUS_VKEY { + static const KMX_DWORD IDENT = LDML_SECTIONID_VKEY; COMP_KMXPLUS_HEADER header; KMX_DWORD count; KMX_DWORD reserved; COMP_KMXPLUS_VKEY_ENTRY entries[]; + /** + * @brief True if section is valid. + */ + bool valid(KMX_DWORD length) const; }; static_assert(sizeof(struct COMP_KMXPLUS_VKEY) % 0x10 == 0, "Structs prior to entries[] should align to 128-bit boundary"); static_assert(sizeof(struct COMP_KMXPLUS_VKEY) == LDML_LENGTH_VKEY, "mismatched size of section vkey"); /** - * See above + * @brief helper accessor object for + * + */ +class kmx_plus { + public: + /** + * @brief Construct a new kmx_plus object. + * Caller must preserve the keyboard file memory + * until after this object is destroyed. + * + * @param keyboard the KMXPlus data + */ + kmx_plus(const COMP_KEYBOARD *keyboard); + const COMP_KMXPLUS_KEYS *keys; + const COMP_KMXPLUS_LOCA *loca; + const COMP_KMXPLUS_META *meta; + const COMP_KMXPLUS_SECT *sect; + const COMP_KMXPLUS_STRS *strs; + const COMP_KMXPLUS_VKEY *vkey; + inline bool is_valid() { return valid; } + private: + bool valid; // true if valid +}; + +/** + * See above - undo workaround for variable arrays */ #if defined(_WIN32) #pragma warning ( default : 4200 ) #endif - -/** - * @brief Validate that this data is the named section. - * - * @param data raw data - * @param ident 4-byte section type - * @return COMP_KMXPLUS_ALLDATA* or null - */ -static inline const COMP_KMXPLUS_HEADER * -validate_as_section(const uint8_t *data, uint32_t ident) { - if (!data) { - return nullptr; - } - const COMP_KMXPLUS_HEADER *all = reinterpret_cast(data); - if (ident != all->ident || (all->size < LDML_LENGTH_HEADER)) { - return nullptr; // invalid header or wrong section - } - return all; -} - -/** - * cast raw data to section - * @return section data or null on error - */ -static inline const COMP_KMXPLUS_SECT * -as_kmxplus_sect(const uint8_t *data) { - const COMP_KMXPLUS_HEADER *all = validate_as_section(data, LDML_SECTIONID_SECT); - return reinterpret_cast(all); -} - -/** - * cast raw data to section - * @return section data or null on error - */ -static inline const COMP_KMXPLUS_STRS * -as_kmxplus_strs(const uint8_t *data) { - const COMP_KMXPLUS_HEADER *all = validate_as_section(data, LDML_SECTIONID_STRS); - return reinterpret_cast(all); -} - -/** - * cast raw data to section - * @return section data or null on error - */ -static inline const COMP_KMXPLUS_KEYS * -as_kmxplus_keys(const uint8_t *data) { - const COMP_KMXPLUS_HEADER *all = validate_as_section(data, LDML_SECTIONID_KEYS); - return reinterpret_cast(all); -} -/** - * cast raw data to section - * @return section data or null on error - */ -static inline const COMP_KMXPLUS_LOCA * -as_kmxplus_loca(const uint8_t *data) { - const COMP_KMXPLUS_HEADER *all = validate_as_section(data, LDML_SECTIONID_LOCA); - return reinterpret_cast(all); -} -/** - * cast raw data to section - * @return section data or null on error - */ -static inline const COMP_KMXPLUS_META * -as_kmxplus_meta(const uint8_t *data) { - const COMP_KMXPLUS_HEADER *all = validate_as_section(data, LDML_SECTIONID_META); - return reinterpret_cast(all); -} -/** - * convert raw data to section - * @return section data or null on error - */ -static inline const COMP_KMXPLUS_VKEY * -as_kmxplus_vkey(const uint8_t *data) { - const COMP_KMXPLUS_HEADER *all = validate_as_section(data, LDML_SECTIONID_VKEY); - return reinterpret_cast(all); -} - -/** - * @param kmxplusdata data from the beginning of the KMXPlus section - * @return true if valid - */ -bool validate_kmxplus_data(const uint8_t *kmxplusdata); - -/** - * @param keyboard pointer to PCOMP_KEYBOARD with plus data following - * @return true if valid - */ -bool validate_kmxplus_data(kmx::PCOMP_KEYBOARD keyboard); - } // namespace kmx } // namespace kbp } // namespace km diff --git a/core/src/ldml/ldml_processor.cpp b/core/src/ldml/ldml_processor.cpp index 8ac18646a2..fa9780e806 100644 --- a/core/src/ldml/ldml_processor.cpp +++ b/core/src/ldml/ldml_processor.cpp @@ -83,6 +83,8 @@ ldml_processor::ldml_processor(path const & kb_path, const std::vector ), _valid(false), vkey_to_string() { +// TODO-LDML: move these asserts into kmx_plus + /** * Assert something, just in this function * If fails, print a message and exit as invalid @@ -93,71 +95,78 @@ ldml_processor::ldml_processor(path const & kb_path, const std::vector _valid = false; \ return; \ } - const kmx::COMP_KMXPLUS_SECT *sect = nullptr; - const kmx::COMP_KMXPLUS_STRS *strs = nullptr; - const kmx::COMP_KMXPLUS_KEYS *keys = nullptr; +// const kmx::COMP_KMXPLUS_SECT *sect = nullptr; +// const kmx::COMP_KMXPLUS_STRS *strs = nullptr; +// const kmx::COMP_KMXPLUS_KEYS *keys = nullptr; - // TODO-LDML: load the file from the buffer (KMXPlus format) - // Note: kb_path is essentially debug metadata here +// // TODO-LDML: load the file from the buffer (KMXPlus format) +// // Note: kb_path is essentially debug metadata here KMXPLUS_ASSERT(true, data.size() > sizeof(kmx::COMP_KEYBOARD_EX)); - // Locate the structs here, but still retain ptrs to the raw structs. +// // Locate the structs here, but still retain ptrs to the raw structs. const kmx::COMP_KEYBOARD* comp_keyboard = (kmx::COMP_KEYBOARD*)data.data(); - KMXPLUS_ASSERT(true, !!(comp_keyboard->dwFlags & KF_KMXPLUS)); - const kmx::COMP_KEYBOARD_EX* ex = reinterpret_cast(comp_keyboard); - // validate size and offset - KMXPLUS_ASSERT(true, ex->kmxplus.dwKMXPlusSize >= LDML_LENGTH_SECT); - KMXPLUS_ASSERT(true, ex->kmxplus.dpKMXPlus >= sizeof(kmx::COMP_KEYBOARD_EX)); - KMXPLUS_ASSERT(true, ex->kmxplus.dpKMXPlus + ex->kmxplus.dwKMXPlusSize <= data.size()); + kmx::kmx_plus kplus(comp_keyboard); + KMXPLUS_ASSERT(true, kplus.is_valid()); - // calculate pointers to start and end of kmxplus - const uint8_t * const kmxplusdata = data.data() + ex->kmxplus.dpKMXPlus; // Start of + data - // const uint8_t* kmxpluslimit = kmxplusdata + ex->kmxplus.dwKMXPlusSize; // End of + data - // Now load sections. + // KMXPLUS_ASSERT(true, !!(comp_keyboard->dwFlags & KF_KMXPLUS)); +// const kmx::COMP_KEYBOARD_EX* ex = reinterpret_cast(comp_keyboard); - // This will validate (and possibly print) all data. - // TODO-LDML: we will be replacing this validation with validation-as-we-go below. - KMXPLUS_ASSERT(true, kmx::validate_kmxplus_data(kmxplusdata)); +// // validate size and offset +// KMXPLUS_ASSERT(true, ex->kmxplus.dwKMXPlusSize >= LDML_LENGTH_SECT); +// KMXPLUS_ASSERT(true, ex->kmxplus.dpKMXPlus >= sizeof(kmx::COMP_KEYBOARD_EX)); +// KMXPLUS_ASSERT(true, ex->kmxplus.dpKMXPlus + ex->kmxplus.dwKMXPlusSize <= data.size()); - // Get out the SECT header - { - const KMX_DWORD offset = 0; - sect = kmx::as_kmxplus_sect(kmxplusdata+offset); - KMXPLUS_ASSERT(true, sect != nullptr); - // Specified data size fits in total - KMXPLUS_ASSERT(true, (offset+sect->header.size) <= ex->kmxplus.dwKMXPlusSize); - } +// // calculate pointers to start and end of kmxplus +// const uint8_t * const kmxplusdata = data.data() + ex->kmxplus.dpKMXPlus; // Start of + data +// // const uint8_t* kmxpluslimit = kmxplusdata + ex->kmxplus.dwKMXPlusSize; // End of + data - // Fill out the other sections we need. - { - const KMX_DWORD offset = sect->find(LDML_SECTIONID_STRS); - KMXPLUS_ASSERT(true, offset != 0); - KMXPLUS_ASSERT(true, offset < ex->kmxplus.dwKMXPlusSize); - strs = kmx::as_kmxplus_strs(kmxplusdata+offset); - KMXPLUS_ASSERT(true, strs != nullptr); - // Specified data size fits in total - KMXPLUS_ASSERT(true, (offset+strs->header.size) <= ex->kmxplus.dwKMXPlusSize); - } +// // Now load sections. - { - const KMX_DWORD offset = sect->find(LDML_SECTIONID_KEYS); - KMXPLUS_ASSERT(true, offset != 0); - keys = kmx::as_kmxplus_keys(kmxplusdata+offset); - KMXPLUS_ASSERT(true, keys != nullptr); - // Specified data size fits in total - KMXPLUS_ASSERT(true, (offset+keys->header.size) <= ex->kmxplus.dwKMXPlusSize); +// // This will validate (and possibly print) all data. +// // TODO-LDML: we will be replacing this validation with validation-as-we-go below. +// KMXPLUS_ASSERT(true, kmx::validate_kmxplus_data(kmxplusdata)); +// // Get out the SECT header +// { +// const KMX_DWORD offset = 0; +// sect = kmx::as_kmxplus_sect(kmxplusdata+offset); +// KMXPLUS_ASSERT(true, sect != nullptr); +// // Specified data size fits in total +// KMXPLUS_ASSERT(true, (offset+sect->header.size) <= ex->kmxplus.dwKMXPlusSize); +// } + +// // Fill out the other sections we need. +// { +// const KMX_DWORD offset = sect->find(LDML_SECTIONID_STRS); +// KMXPLUS_ASSERT(true, offset != 0); +// KMXPLUS_ASSERT(true, offset < ex->kmxplus.dwKMXPlusSize); +// strs = kmx::as_kmxplus_strs(kmxplusdata+offset); +// KMXPLUS_ASSERT(true, strs != nullptr); +// // Specified data size fits in total +// KMXPLUS_ASSERT(true, (offset+strs->header.size) <= ex->kmxplus.dwKMXPlusSize); +// } + +// { +// const KMX_DWORD offset = sect->find(LDML_SECTIONID_KEYS); +// KMXPLUS_ASSERT(true, offset != 0); +// keys = kmx::as_kmxplus_keys(kmxplusdata+offset); +// KMXPLUS_ASSERT(true, keys != nullptr); +// // Specified data size fits in total +// KMXPLUS_ASSERT(true, (offset+keys->header.size) <= ex->kmxplus.dwKMXPlusSize); + if (kplus.keys != nullptr) { // read all keys into array - for (KMX_DWORD i=0; icount; i++) { - const kmx::COMP_KMXPLUS_KEYS_ENTRY &entry = keys->entries[i]; + for (KMX_DWORD i=0; icount; i++) { + const kmx::COMP_KMXPLUS_KEYS_ENTRY &entry = kplus.keys->entries[i]; KMX_DWORD len = 0; KMX_WCHAR out[BUFSIZ]; if (entry.flags && LDML_KEYS_FLAGS_EXTEND) { - KMXPLUS_ASSERT(false, nullptr == strs->get(entry.to, out, BUFSIZ)); + KMXPLUS_ASSERT(false, nullptr == kplus.strs); // need a string table to get strings + KMXPLUS_ASSERT(false, nullptr == kplus.strs->get(entry.to, out, BUFSIZ)); for(len=0; len ldml_vkey_id vkey_id((km_kbp_virtual_key)entry.vkey, (uint16_t)entry.mod); vkey_to_string[vkey_id] = std::u16string(out, len); // assign the string } - } + } // else: no keys! KMXPLUS_PRINTLN("_valid = true"); #undef KMXPLUS_ASSERT From 3684cc43d071b7bf3b84bbdbf7e1153fd6b7ab79 Mon Sep 17 00:00:00 2001 From: "Steven R. Loomis" Date: Wed, 31 Aug 2022 16:48:04 -0500 Subject: [PATCH 2/3] =?UTF-8?q?fix(core):=20fix=20validators=20?= =?UTF-8?q?=F0=9F=99=80?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit yes, a kmxplus file does not start with a 'sect' ! #7136 --- core/src/kmx/kmx_plus.cpp | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/core/src/kmx/kmx_plus.cpp b/core/src/kmx/kmx_plus.cpp index 86fddc4460..570e6e125a 100644 --- a/core/src/kmx/kmx_plus.cpp +++ b/core/src/kmx/kmx_plus.cpp @@ -274,7 +274,7 @@ kmx_plus::kmx_plus(const COMP_KEYBOARD *keyboard) KMXPLUS_PRINTF(("kmx_plus(): KMXPlus offset 0x%X, KMXPlus size 0x%X\n", ex->kmxplus.dpKMXPlus, ex->kmxplus.dwKMXPlusSize)); const uint8_t* rawdata = reinterpret_cast(keyboard); - sect = section_from_bytes(rawdata, ex->kmxplus.dwKMXPlusSize); + sect = section_from_bytes(rawdata+ex->kmxplus.dpKMXPlus, ex->kmxplus.dwKMXPlusSize); if (sect == nullptr) { KMXPLUS_PRINTF(("kmx_plus(): 'sect' did not validate\n")); valid = false; From f3f498e9232f5e412a34986d2848488aa6979c90 Mon Sep 17 00:00:00 2001 From: "Steven R. Loomis" Date: Wed, 31 Aug 2022 17:01:12 -0500 Subject: [PATCH 3/3] =?UTF-8?q?fix(core):=20fix=20validators=20?= =?UTF-8?q?=F0=9F=99=80?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit - copy over asserts from 'old' code #7136 --- core/src/kmx/kmx_plus.cpp | 22 ++++++++++-- core/src/kmx/kmx_plus.h | 5 +-- core/src/ldml/ldml_processor.cpp | 57 +++----------------------------- 3 files changed, 26 insertions(+), 58 deletions(-) diff --git a/core/src/kmx/kmx_plus.cpp b/core/src/kmx/kmx_plus.cpp index 570e6e125a..f368b6d483 100644 --- a/core/src/kmx/kmx_plus.cpp +++ b/core/src/kmx/kmx_plus.cpp @@ -89,6 +89,10 @@ header_from_bytes(const uint8_t *data, KMX_DWORD length, uint32_t ident) { */ template const T *section_from_bytes(const uint8_t *data, KMX_DWORD length) { + if (length < sizeof(T)) { // Does not include dynamic data. First check. + debug_println("length < sizeof(section)"); + return nullptr; + } const COMP_KMXPLUS_HEADER *header = header_from_bytes(data, length, T::IDENT); const T *section = reinterpret_cast(header); if (section != nullptr && section->valid(length)) { @@ -259,19 +263,30 @@ COMP_KMXPLUS_SECT::valid(KMX_DWORD length) const { return overall_valid; } -kmx_plus::kmx_plus(const COMP_KEYBOARD *keyboard) +kmx_plus::kmx_plus(const COMP_KEYBOARD *keyboard, size_t length) : keys(nullptr), loca(nullptr), meta(nullptr), sect(nullptr), strs(nullptr), vkey(nullptr), valid(false) { KMXPLUS_PRINTF(("kmx_plus(): Got a COMP_KEYBOARD at %p\n", keyboard)); if (!(keyboard->dwFlags & KF_KMXPLUS)) { - KMXPLUS_PRINTF(("Err: flags KF_KMXPLUS not set\n")); + KMXPLUS_PRINTF(("Err: flags COMP_KEYBOARD.dwFlags did not have KF_KMXPLUS set\n")); valid = false; return; } const COMP_KEYBOARD_EX* ex = reinterpret_cast(keyboard); KMXPLUS_PRINTF(("kmx_plus(): KMXPlus offset 0x%X, KMXPlus size 0x%X\n", ex->kmxplus.dpKMXPlus, ex->kmxplus.dwKMXPlusSize)); + if (ex->kmxplus.dpKMXPlus < sizeof(kmx::COMP_KEYBOARD_EX)) { + debug_println("dwKMXPlus is not past the end of COMP_KEYBOARD_EX"); + valid = false; + return; + } + if ( ex->kmxplus.dpKMXPlus + ex->kmxplus.dwKMXPlusSize > length) { + debug_println("dpKMXPlus + dwKMXPlusSize is past the end of the file"); + valid = false; + return; + } + const uint8_t* rawdata = reinterpret_cast(keyboard); sect = section_from_bytes(rawdata+ex->kmxplus.dpKMXPlus, ex->kmxplus.dwKMXPlusSize); @@ -280,7 +295,8 @@ kmx_plus::kmx_plus(const COMP_KEYBOARD *keyboard) valid = false; } else { valid = true; - // load other sections + // load other sections, validating as we go + // these will be nullptr if they don't validate keys = section_from_sect(sect); loca = section_from_sect(sect); meta = section_from_sect(sect); diff --git a/core/src/kmx/kmx_plus.h b/core/src/kmx/kmx_plus.h index e9033df0ba..3510beeb67 100644 --- a/core/src/kmx/kmx_plus.h +++ b/core/src/kmx/kmx_plus.h @@ -192,9 +192,10 @@ class kmx_plus { * Caller must preserve the keyboard file memory * until after this object is destroyed. * - * @param keyboard the KMXPlus data + * @param keyboard the KMX data + * @param length length of the entire KMX file */ - kmx_plus(const COMP_KEYBOARD *keyboard); + kmx_plus(const COMP_KEYBOARD *keyboard, size_t length); const COMP_KMXPLUS_KEYS *keys; const COMP_KMXPLUS_LOCA *loca; const COMP_KMXPLUS_META *meta; diff --git a/core/src/ldml/ldml_processor.cpp b/core/src/ldml/ldml_processor.cpp index fa9780e806..a473681241 100644 --- a/core/src/ldml/ldml_processor.cpp +++ b/core/src/ldml/ldml_processor.cpp @@ -95,66 +95,17 @@ ldml_processor::ldml_processor(path const & kb_path, const std::vector _valid = false; \ return; \ } -// const kmx::COMP_KMXPLUS_SECT *sect = nullptr; -// const kmx::COMP_KMXPLUS_STRS *strs = nullptr; -// const kmx::COMP_KMXPLUS_KEYS *keys = nullptr; -// // TODO-LDML: load the file from the buffer (KMXPlus format) -// // Note: kb_path is essentially debug metadata here KMXPLUS_ASSERT(true, data.size() > sizeof(kmx::COMP_KEYBOARD_EX)); // // Locate the structs here, but still retain ptrs to the raw structs. const kmx::COMP_KEYBOARD* comp_keyboard = (kmx::COMP_KEYBOARD*)data.data(); - kmx::kmx_plus kplus(comp_keyboard); + kmx::kmx_plus kplus(comp_keyboard, data.size()); + KMXPLUS_ASSERT(true, kplus.is_valid()); - - // KMXPLUS_ASSERT(true, !!(comp_keyboard->dwFlags & KF_KMXPLUS)); -// const kmx::COMP_KEYBOARD_EX* ex = reinterpret_cast(comp_keyboard); - -// // validate size and offset -// KMXPLUS_ASSERT(true, ex->kmxplus.dwKMXPlusSize >= LDML_LENGTH_SECT); -// KMXPLUS_ASSERT(true, ex->kmxplus.dpKMXPlus >= sizeof(kmx::COMP_KEYBOARD_EX)); -// KMXPLUS_ASSERT(true, ex->kmxplus.dpKMXPlus + ex->kmxplus.dwKMXPlusSize <= data.size()); - -// // calculate pointers to start and end of kmxplus -// const uint8_t * const kmxplusdata = data.data() + ex->kmxplus.dpKMXPlus; // Start of + data -// // const uint8_t* kmxpluslimit = kmxplusdata + ex->kmxplus.dwKMXPlusSize; // End of + data - -// // Now load sections. - -// // This will validate (and possibly print) all data. -// // TODO-LDML: we will be replacing this validation with validation-as-we-go below. -// KMXPLUS_ASSERT(true, kmx::validate_kmxplus_data(kmxplusdata)); - -// // Get out the SECT header -// { -// const KMX_DWORD offset = 0; -// sect = kmx::as_kmxplus_sect(kmxplusdata+offset); -// KMXPLUS_ASSERT(true, sect != nullptr); -// // Specified data size fits in total -// KMXPLUS_ASSERT(true, (offset+sect->header.size) <= ex->kmxplus.dwKMXPlusSize); -// } - -// // Fill out the other sections we need. -// { -// const KMX_DWORD offset = sect->find(LDML_SECTIONID_STRS); -// KMXPLUS_ASSERT(true, offset != 0); -// KMXPLUS_ASSERT(true, offset < ex->kmxplus.dwKMXPlusSize); -// strs = kmx::as_kmxplus_strs(kmxplusdata+offset); -// KMXPLUS_ASSERT(true, strs != nullptr); -// // Specified data size fits in total -// KMXPLUS_ASSERT(true, (offset+strs->header.size) <= ex->kmxplus.dwKMXPlusSize); -// } - -// { -// const KMX_DWORD offset = sect->find(LDML_SECTIONID_KEYS); -// KMXPLUS_ASSERT(true, offset != 0); -// keys = kmx::as_kmxplus_keys(kmxplusdata+offset); -// KMXPLUS_ASSERT(true, keys != nullptr); -// // Specified data size fits in total -// KMXPLUS_ASSERT(true, (offset+keys->header.size) <= ex->kmxplus.dwKMXPlusSize); + // Now, if we have keys, use them. if (kplus.keys != nullptr) { // read all keys into array for (KMX_DWORD i=0; icount; i++) { @@ -186,7 +137,7 @@ ldml_processor::ldml_processor(path const & kb_path, const std::vector ldml_vkey_id vkey_id((km_kbp_virtual_key)entry.vkey, (uint16_t)entry.mod); vkey_to_string[vkey_id] = std::u16string(out, len); // assign the string } - } // else: no keys! + } // else: no keys! but still valid. Just, no keys. KMXPLUS_PRINTLN("_valid = true"); #undef KMXPLUS_ASSERT