From 7e4fdf7669dcef193f27008a3022e3ab09904468 Mon Sep 17 00:00:00 2001 From: x1nixmzeng Date: Sat, 9 Jan 2016 01:19:05 +0000 Subject: [PATCH 1/3] Fixed rare crash when accessing library import by name The library name index may only be 8-bits. This bug was not present in the previous implementation due to a bitmask (0xFF) - see https://github.com/benvanik/xenia/blob/ea99ba8e3bbac03386c95efe1496d5f4b03f5456/src/xenia/kernel/util/xex2.cc#L272 --- src/xenia/cpu/xex_module.cc | 6 ++++-- src/xenia/kernel/user_module.cc | 5 +++-- src/xenia/kernel/util/xex2.cc | 3 ++- 3 files changed, 9 insertions(+), 5 deletions(-) diff --git a/src/xenia/cpu/xex_module.cc b/src/xenia/cpu/xex_module.cc index 860b204b5..4d5e6801b 100644 --- a/src/xenia/cpu/xex_module.cc +++ b/src/xenia/cpu/xex_module.cc @@ -260,10 +260,12 @@ bool XexModule::Load(const std::string& name, const std::string& path, auto libraries_ptr = reinterpret_cast(opt_import_header) + opt_import_header->string_table_size + 12; uint32_t library_offset = 0; - for (uint32_t i = 0; i < opt_import_header->library_count; i++) { + uint32_t library_count = opt_import_header->library_count; + for (uint32_t i = 0; i < library_count; i++) { auto library = reinterpret_cast(libraries_ptr + library_offset); - SetupLibraryImports(string_table[library->name_index], library); + SetupLibraryImports(string_table[library->name_index % library_count], + library); library_offset += library->size; } diff --git a/src/xenia/kernel/user_module.cc b/src/xenia/kernel/user_module.cc index 07894984d..b831c8adb 100644 --- a/src/xenia/kernel/user_module.cc +++ b/src/xenia/kernel/user_module.cc @@ -475,10 +475,11 @@ void UserModule::Dump() { reinterpret_cast(opt_import_libraries) + opt_import_libraries->string_table_size + 12; uint32_t library_offset = 0; - for (uint32_t l = 0; l < opt_import_libraries->library_count; l++) { + uint32_t library_count = opt_import_libraries->library_count; + for (uint32_t l = 0; l < library_count; l++) { auto library = reinterpret_cast( libraries + library_offset); - auto name = string_table[library->name_index]; + auto name = string_table[library->name_index % library_count]; sb.AppendFormat(" %s - %d imports\n", name, (uint16_t)library->count); diff --git a/src/xenia/kernel/util/xex2.cc b/src/xenia/kernel/util/xex2.cc index dbbea04f3..2bfe11ca3 100644 --- a/src/xenia/kernel/util/xex2.cc +++ b/src/xenia/kernel/util/xex2.cc @@ -285,7 +285,8 @@ int xe_xex2_read_header(const uint8_t* addr, const size_t length, library->version.value = src_library->version.value; library->min_version.value = src_library->version_min.value; - std::strncpy(library->name, string_table[src_library->name_index], + std::strncpy(library->name, + string_table[src_library->name_index % count], xe::countof(library->name)); library->record_count = src_library->count; From 06f259c87daab3105707e3fd42ce4884264e8670 Mon Sep 17 00:00:00 2001 From: x1nixmzeng Date: Sat, 9 Jan 2016 02:43:29 +0000 Subject: [PATCH 2/3] Mask the name index instead The maximum number of import libraries (32) is already asserted --- src/xenia/cpu/xex_module.cc | 3 +-- src/xenia/kernel/user_module.cc | 2 +- src/xenia/kernel/util/xex2.cc | 2 +- 3 files changed, 3 insertions(+), 4 deletions(-) diff --git a/src/xenia/cpu/xex_module.cc b/src/xenia/cpu/xex_module.cc index 4d5e6801b..28fdd9b8c 100644 --- a/src/xenia/cpu/xex_module.cc +++ b/src/xenia/cpu/xex_module.cc @@ -264,8 +264,7 @@ bool XexModule::Load(const std::string& name, const std::string& path, for (uint32_t i = 0; i < library_count; i++) { auto library = reinterpret_cast(libraries_ptr + library_offset); - SetupLibraryImports(string_table[library->name_index % library_count], - library); + SetupLibraryImports(string_table[library->name_index & 0xFF], library); library_offset += library->size; } diff --git a/src/xenia/kernel/user_module.cc b/src/xenia/kernel/user_module.cc index b831c8adb..36e11470f 100644 --- a/src/xenia/kernel/user_module.cc +++ b/src/xenia/kernel/user_module.cc @@ -479,7 +479,7 @@ void UserModule::Dump() { for (uint32_t l = 0; l < library_count; l++) { auto library = reinterpret_cast( libraries + library_offset); - auto name = string_table[library->name_index % library_count]; + auto name = string_table[library->name_index & 0xFF]; sb.AppendFormat(" %s - %d imports\n", name, (uint16_t)library->count); diff --git a/src/xenia/kernel/util/xex2.cc b/src/xenia/kernel/util/xex2.cc index 2bfe11ca3..823ec22f3 100644 --- a/src/xenia/kernel/util/xex2.cc +++ b/src/xenia/kernel/util/xex2.cc @@ -286,7 +286,7 @@ int xe_xex2_read_header(const uint8_t* addr, const size_t length, library->min_version.value = src_library->version_min.value; std::strncpy(library->name, - string_table[src_library->name_index % count], + string_table[src_library->name_index & 0xFF], xe::countof(library->name)); library->record_count = src_library->count; From 3eb602c93aded8bc27c93e887e22200c47f2bebf Mon Sep 17 00:00:00 2001 From: x1nixmzeng Date: Sat, 9 Jan 2016 17:55:57 +0000 Subject: [PATCH 3/3] Assert library name index is in range of the string table As suggested by @DrChat --- src/xenia/cpu/xex_module.cc | 12 ++++++++---- 1 file changed, 8 insertions(+), 4 deletions(-) diff --git a/src/xenia/cpu/xex_module.cc b/src/xenia/cpu/xex_module.cc index 28fdd9b8c..2753fc698 100644 --- a/src/xenia/cpu/xex_module.cc +++ b/src/xenia/cpu/xex_module.cc @@ -242,13 +242,15 @@ bool XexModule::Load(const std::string& name, const std::string& path, // FIXME: Don't know if 32 is the actual limit, but haven't seen more than 2. const char* string_table[32]; std::memset(string_table, 0, sizeof(string_table)); + size_t max_string_table_index = 0; // Parse the string table - for (size_t i = 0, j = 0; i < opt_import_header->string_table_size; j++) { - assert_true(j < xe::countof(string_table)); + for (size_t i = 0; i < opt_import_header->string_table_size; + ++max_string_table_index) { + assert_true(max_string_table_index < xe::countof(string_table)); const char* str = opt_import_header->string_table + i; - string_table[j] = str; + string_table[max_string_table_index] = str; i += std::strlen(str) + 1; // Padding @@ -264,7 +266,9 @@ bool XexModule::Load(const std::string& name, const std::string& path, for (uint32_t i = 0; i < library_count; i++) { auto library = reinterpret_cast(libraries_ptr + library_offset); - SetupLibraryImports(string_table[library->name_index & 0xFF], library); + size_t library_name_index = library->name_index & 0xFF; + assert_true(library_name_index < max_string_table_index); + SetupLibraryImports(string_table[library_name_index], library); library_offset += library->size; }