llvmorg-github-actions[bot] wrote:
<!--LLVM PR SUMMARY COMMENT--> @llvm/pr-subscribers-lldb Author: Yao Qi (qiyao) <details> <summary>Changes</summary> Every function in `ObjectFileMachO` and `ObjectContainerMachOFileset` that iterates over load commands advances the file offset by `lc.cmdsize` after reading each command. A malformed command with cmdsize smaller than `sizeof(load_command)` (in particular cmdsize = 0) does not make forward progress, so the loop spins for ncmds iterations. With `ncmds` close to `INT_MAX` the function never returns in practice. Factor the read-and-validate step into a static template helper `ReadMachOCommand<T>` in each plugin's translation unit. It reads the 8-byte cmd/cmdsize header and returns false on EOF or on a cmdsize that is too small to make forward progress. All load-command loops now use this helper, replacing the previously duplicated GetU32 + cmdsize check. `T` may be `llvm::MachO::load_command` or any of its richer variants (uuid_command, dylib_command, thread_command, ident_command, encryption_info_command, ...). The helper only touches the leading cmd/cmdsize fields, leaving the rest of `T` for the caller to fill in. Affected loops in `ObjectFileMachO`: IsStripped, GetEncryptedFileRanges, CreateSections, ParseSymtab, GetUUID (static), GetAllArchSpecs (two loops), GetDependentModules, GetEntryPointAddress, GetNumThreadContexts, FindLC_NOTEByName, GetIdentifierString, GetVersion, FindMinimumVersionInfo And in ObjectContainerMachOFileset: ParseFileset Add unit tests (`ObjectFileMachOTest::ZeroCmdSize` and `ObjectContainerMachOFilesetTest::ZeroCmdSize`) that feed a 40-byte Mach-O with `ncmds = 0x7FFFFFFF` and `cmdsize = 0` into the relevant parsers. Without the fix the tests spin ~2 billion iterations; with the fix they return immediately. Found by lldb-target-fuzzer. Assisted-by: Claude --- Full diff: https://github.com/llvm/llvm-project/pull/205134.diff 5 Files Affected: - (modified) lldb/source/Plugins/ObjectContainer/Mach-O-Fileset/ObjectContainerMachOFileset.cpp (+19-1) - (modified) lldb/source/Plugins/ObjectFile/Mach-O/ObjectFileMachO.cpp (+32-14) - (modified) lldb/unittests/ObjectContainer/CMakeLists.txt (+1) - (modified) lldb/unittests/ObjectContainer/ObjectContainerUniversalMachOTest.cpp (+72) - (modified) lldb/unittests/ObjectFile/MachO/TestObjectFileMachO.cpp (+65) ``````````diff diff --git a/lldb/source/Plugins/ObjectContainer/Mach-O-Fileset/ObjectContainerMachOFileset.cpp b/lldb/source/Plugins/ObjectContainer/Mach-O-Fileset/ObjectContainerMachOFileset.cpp index 706b8e38e9510..fb6b3de6a0c81 100644 --- a/lldb/source/Plugins/ObjectContainer/Mach-O-Fileset/ObjectContainerMachOFileset.cpp +++ b/lldb/source/Plugins/ObjectContainer/Mach-O-Fileset/ObjectContainerMachOFileset.cpp @@ -21,6 +21,24 @@ using namespace lldb; using namespace lldb_private; using namespace llvm::MachO; +/// Read a Mach-O load-command header (cmd + cmdsize) from \p data at +/// \p offset into \p cmd, advancing \p offset by 8 bytes. \p T may be +/// \c llvm::MachO::load_command or any of its richer variants +/// (\c thread_command, \c dylib_command, \c encryption_info_command, ...); +/// only the leading cmd/cmdsize fields are touched by this read. Returns +/// false on EOF or on a cmdsize smaller than sizeof(load_command), in which +/// case callers should break out of their load-command loop to avoid spinning +/// on malformed input. +template <typename T> +static bool ReadMachOCommand(DataExtractor &data, lldb::offset_t &offset, + T &cmd) { + if (data.GetU32(&offset, &cmd, 2) == nullptr) + return false; + if (cmd.cmdsize < sizeof(load_command)) + return false; + return true; +} + LLDB_PLUGIN_DEFINE(ObjectContainerMachOFileset) void ObjectContainerMachOFileset::Initialize() { @@ -142,7 +160,7 @@ ParseFileset(DataExtractor &extractor, mach_header header, for (uint32_t i = 0; i < header.ncmds; ++i) { const lldb::offset_t load_cmd_offset = offset; load_command lc = {}; - if (extractor.GetU32(&offset, &lc.cmd, 2) == nullptr) + if (!ReadMachOCommand(extractor, offset, lc)) break; // If we know the load address we can compute the slide. diff --git a/lldb/source/Plugins/ObjectFile/Mach-O/ObjectFileMachO.cpp b/lldb/source/Plugins/ObjectFile/Mach-O/ObjectFileMachO.cpp index a7096fa83315f..a4b23c162d194 100644 --- a/lldb/source/Plugins/ObjectFile/Mach-O/ObjectFileMachO.cpp +++ b/lldb/source/Plugins/ObjectFile/Mach-O/ObjectFileMachO.cpp @@ -139,6 +139,24 @@ static constexpr llvm::StringLiteral g_executable_path = "@executable_path"; LLDB_PLUGIN_DEFINE(ObjectFileMachO) +/// Read a Mach-O load-command header (cmd + cmdsize) from \p data at +/// \p offset into \p cmd, advancing \p offset by 8 bytes. \p T may be +/// \c llvm::MachO::load_command or any of its richer variants +/// (\c thread_command, \c dylib_command, \c encryption_info_command, ...); +/// only the leading cmd/cmdsize fields are touched by this read. Returns +/// false on EOF or on a cmdsize smaller than sizeof(load_command), in which +/// case callers should break out of their load-command loop to avoid spinning +/// on malformed input. +template <typename T> +static bool ReadMachOCommand(const DataExtractor &data, lldb::offset_t &offset, + T &cmd) { + if (data.GetU32(&offset, &cmd, 2) == nullptr) + return false; + if (cmd.cmdsize < sizeof(load_command)) + return false; + return true; +} + static void PrintRegisterValue(RegisterContext *reg_ctx, const char *name, const char *alt_name, size_t reg_byte_size, Stream &data) { @@ -1305,7 +1323,7 @@ bool ObjectFileMachO::IsStripped() { const lldb::offset_t load_cmd_offset = offset; llvm::MachO::load_command lc = {}; - if (m_data_nsp->GetU32(&offset, &lc.cmd, 2) == nullptr) + if (!ReadMachOCommand(*m_data_nsp, offset, lc)) break; if (lc.cmd == LC_DYSYMTAB) { m_dysymtab.cmd = lc.cmd; @@ -1334,7 +1352,7 @@ ObjectFileMachO::EncryptedFileRanges ObjectFileMachO::GetEncryptedFileRanges() { llvm::MachO::encryption_info_command encryption_cmd; for (uint32_t i = 0; i < m_header.ncmds; ++i) { const lldb::offset_t load_cmd_offset = offset; - if (m_data_nsp->GetU32(&offset, &encryption_cmd, 2) == nullptr) + if (!ReadMachOCommand(*m_data_nsp, offset, encryption_cmd)) break; // LC_ENCRYPTION_INFO and LC_ENCRYPTION_INFO_64 have the same sizes for the @@ -1883,7 +1901,7 @@ void ObjectFileMachO::CreateSections(SectionList &unified_section_list) { llvm::MachO::load_command load_cmd; for (uint32_t i = 0; i < m_header.ncmds; ++i) { const lldb::offset_t load_cmd_offset = offset; - if (m_data_nsp->GetU32(&offset, &load_cmd, 2) == nullptr) + if (!ReadMachOCommand(*m_data_nsp, offset, load_cmd)) break; if (load_cmd.cmd == LC_SEGMENT || load_cmd.cmd == LC_SEGMENT_64) @@ -2116,7 +2134,7 @@ void ObjectFileMachO::ParseSymtab(Symtab &symtab) { const lldb::offset_t cmd_offset = offset; // Read in the load command and load command size llvm::MachO::load_command lc; - if (m_data_nsp->GetU32(&offset, &lc, 2) == nullptr) + if (!ReadMachOCommand(*m_data_nsp, offset, lc)) break; // Watch for the symbol table load command switch (lc.cmd) { @@ -4470,7 +4488,7 @@ UUID ObjectFileMachO::GetUUID(const llvm::MachO::mach_header &header, lldb::offset_t offset = lc_offset; for (i = 0; i < header.ncmds; ++i) { const lldb::offset_t cmd_offset = offset; - if (data.GetU32(&offset, &load_cmd, 2) == nullptr) + if (!ReadMachOCommand(data, offset, load_cmd)) break; if (load_cmd.cmd == LC_UUID) { @@ -4629,7 +4647,7 @@ void ObjectFileMachO::GetAllArchSpecs(const llvm::MachO::mach_header &header, lldb::offset_t offset = lc_offset; for (uint32_t i = 0; i < header.ncmds; ++i) { const lldb::offset_t cmd_offset = offset; - if (data.GetU32(&offset, &load_cmd, 2) == nullptr) + if (!ReadMachOCommand(data, offset, load_cmd)) break; llvm::MachO::version_min_command version_min; @@ -4679,7 +4697,7 @@ void ObjectFileMachO::GetAllArchSpecs(const llvm::MachO::mach_header &header, offset = lc_offset; for (uint32_t i = 0; i < header.ncmds; ++i) { const lldb::offset_t cmd_offset = offset; - if (data.GetU32(&offset, &load_cmd, 2) == nullptr) + if (!ReadMachOCommand(data, offset, load_cmd)) break; do { @@ -4765,7 +4783,7 @@ uint32_t ObjectFileMachO::GetDependentModules(FileSpecList &files) { uint32_t i; for (i = 0; i < m_header.ncmds; ++i) { const uint32_t cmd_offset = offset; - if (m_data_nsp->GetU32(&offset, &load_cmd, 2) == nullptr) + if (!ReadMachOCommand(*m_data_nsp, offset, load_cmd)) break; switch (load_cmd.cmd) { @@ -4914,7 +4932,7 @@ lldb_private::Address ObjectFileMachO::GetEntryPointAddress() { for (i = 0; i < m_header.ncmds; ++i) { const lldb::offset_t cmd_offset = offset; - if (m_data_nsp->GetU32(&offset, &load_cmd, 2) == nullptr) + if (!ReadMachOCommand(*m_data_nsp, offset, load_cmd)) break; switch (load_cmd.cmd) { @@ -5054,7 +5072,7 @@ uint32_t ObjectFileMachO::GetNumThreadContexts() { llvm::MachO::thread_command thread_cmd; for (uint32_t i = 0; i < m_header.ncmds; ++i) { const uint32_t cmd_offset = offset; - if (m_data_nsp->GetU32(&offset, &thread_cmd, 2) == nullptr) + if (!ReadMachOCommand(*m_data_nsp, offset, thread_cmd)) break; if (thread_cmd.cmd == LC_THREAD) { @@ -5080,7 +5098,7 @@ ObjectFileMachO::FindLC_NOTEByName(std::string name) { for (uint32_t i = 0; i < m_header.ncmds; ++i) { const uint32_t cmd_offset = offset; llvm::MachO::load_command lc = {}; - if (m_data_nsp->GetU32(&offset, &lc.cmd, 2) == nullptr) + if (!ReadMachOCommand(*m_data_nsp, offset, lc)) break; if (lc.cmd == LC_NOTE) { char data_owner[17]; @@ -5130,7 +5148,7 @@ std::string ObjectFileMachO::GetIdentifierString() { for (uint32_t i = 0; i < m_header.ncmds; ++i) { const uint32_t cmd_offset = offset; llvm::MachO::ident_command ident_command; - if (m_data_nsp->GetU32(&offset, &ident_command, 2) == nullptr) + if (!ReadMachOCommand(*m_data_nsp, offset, ident_command)) break; if (ident_command.cmd == LC_IDENT && ident_command.cmdsize != 0) { std::string result(ident_command.cmdsize, '\0'); @@ -5555,7 +5573,7 @@ llvm::VersionTuple ObjectFileMachO::GetVersion() { uint32_t i; for (i = 0; i < m_header.ncmds; ++i) { const lldb::offset_t cmd_offset = offset; - if (m_data_nsp->GetU32(&offset, &load_cmd, 2) == nullptr) + if (!ReadMachOCommand(*m_data_nsp, offset, load_cmd)) break; if (load_cmd.cmd == LC_ID_DYLIB) { @@ -5715,7 +5733,7 @@ static llvm::VersionTuple FindMinimumVersionInfo(DataExtractor &data, for (size_t i = 0; i < ncmds; i++) { const lldb::offset_t load_cmd_offset = offset; llvm::MachO::load_command lc = {}; - if (data.GetU32(&offset, &lc.cmd, 2) == nullptr) + if (!ReadMachOCommand(data, offset, lc)) break; uint32_t version = 0; diff --git a/lldb/unittests/ObjectContainer/CMakeLists.txt b/lldb/unittests/ObjectContainer/CMakeLists.txt index 80778d1c32f17..8acfccf12f4dd 100644 --- a/lldb/unittests/ObjectContainer/CMakeLists.txt +++ b/lldb/unittests/ObjectContainer/CMakeLists.txt @@ -3,6 +3,7 @@ add_lldb_unittest(ObjectContainerTests LINK_LIBS lldbPluginObjectContainerMachOArchive + lldbPluginObjectContainerMachOFileset lldbCore lldbUtilityHelpers LLVMTestingSupport diff --git a/lldb/unittests/ObjectContainer/ObjectContainerUniversalMachOTest.cpp b/lldb/unittests/ObjectContainer/ObjectContainerUniversalMachOTest.cpp index a4346befbfd8b..374c3c96b1a9f 100644 --- a/lldb/unittests/ObjectContainer/ObjectContainerUniversalMachOTest.cpp +++ b/lldb/unittests/ObjectContainer/ObjectContainerUniversalMachOTest.cpp @@ -7,11 +7,14 @@ //===----------------------------------------------------------------------===// #include "Plugins/ObjectContainer/Universal-Mach-O/ObjectContainerUniversalMachO.h" +#include "Plugins/ObjectContainer/Mach-O-Fileset/ObjectContainerMachOFileset.h" #include "TestingSupport/SubsystemRAII.h" #include "TestingSupport/TestUtilities.h" #include "lldb/Core/ModuleSpec.h" #include "lldb/Host/FileSystem.h" #include "lldb/Symbol/ObjectFile.h" +#include "lldb/Utility/DataBufferHeap.h" +#include "lldb/Utility/DataExtractor.h" #include "lldb/Utility/FileSpec.h" #include "llvm/Support/FileSystem.h" #include "llvm/Testing/Support/Error.h" @@ -117,3 +120,72 @@ TEST_F(ObjectContainerUniversalMachOTest, SliceOffsetZero) { ASSERT_THAT_ERROR(TmpFile->discard(), llvm::Succeeded()); } + +// Regression fixture: a Mach-O fileset whose single load command has +// cmdsize = 0. With ncmds set near INT_MAX the function hangs. The +// fix breaks out of the loop as soon as +// cmdsize < sizeof(load_command). Found by lldb-target-fuzzer. +namespace { +class ObjectContainerMachOFilesetTest : public ::testing::Test { + SubsystemRAII<FileSystem, ObjectContainerMachOFileset> subsystems; +}; +} // namespace + +TEST_F(ObjectContainerMachOFilesetTest, ZeroCmdSize) { + // Minimal little-endian x86_64 Mach-O fileset: mach_header_64 (32 bytes) + // followed by a single load_command with cmdsize = 0. ncmds is set to + // 0x7FFFFFFF so that without the fix ParseFileset spins ~2 billion times and + // never returns in practice; with the fix it breaks on the first iteration. + // Reaching the assertion below is the regression check. + const uint8_t kData[] = { + // mach_header_64 (little-endian) + 0xCF, + 0xFA, + 0xED, + 0xFE, // magic: MH_MAGIC_64 + 0x07, + 0x00, + 0x00, + 0x01, // cputype: CPU_TYPE_X86_64 + 0x03, + 0x00, + 0x00, + 0x80, // cpusubtype: CPU_SUBTYPE_X86_64_ALL + 0x0C, + 0x00, + 0x00, + 0x00, // filetype: MH_FILESET + 0xFF, + 0xFF, + 0xFF, + 0x7F, // ncmds: 0x7FFFFFFF + 0x08, + 0x00, + 0x00, + 0x00, // sizeofcmds: 8 + 0x00, + 0x00, + 0x00, + 0x00, // flags: 0 + 0x00, + 0x00, + 0x00, + 0x00, // reserved: 0 + // load_command + 0x19, + 0x00, + 0x00, + 0x00, // cmd: LC_SEGMENT_64 (arbitrary) + 0x00, + 0x00, + 0x00, + 0x00, // cmdsize: 0 ← causes the spin + }; + auto Buf = std::make_shared<DataBufferHeap>(kData, sizeof(kData)); + lldb::DataExtractorSP DataSP = + std::make_shared<lldb_private::DataExtractor>(Buf, lldb::eByteOrderLittle, + /*addr_size=*/8); + // Before the fix ParseFileset loops ~0x7FFFFFFF times and never returns. + (void)ObjectContainerMachOFileset::GetModuleSpecifications(FileSpec(), DataSP, + 0, sizeof(kData)); +} diff --git a/lldb/unittests/ObjectFile/MachO/TestObjectFileMachO.cpp b/lldb/unittests/ObjectFile/MachO/TestObjectFileMachO.cpp index 147ea55e85efa..70e0b570fca8b 100644 --- a/lldb/unittests/ObjectFile/MachO/TestObjectFileMachO.cpp +++ b/lldb/unittests/ObjectFile/MachO/TestObjectFileMachO.cpp @@ -15,6 +15,9 @@ #include "lldb/Core/ModuleSpec.h" #include "lldb/Host/FileSystem.h" #include "lldb/Host/HostInfo.h" +#include "lldb/Symbol/ObjectFile.h" +#include "lldb/Utility/DataBufferHeap.h" +#include "lldb/Utility/DataExtractor.h" #include "lldb/Utility/FileSpec.h" #include "lldb/lldb-defines.h" #include "gtest/gtest.h" @@ -107,3 +110,65 @@ TEST_F(ObjectFileMachOTest, IndirectSymbolsInTheSharedCache) { OF->ParseSymtab(symtab); } #endif + +// Regression fixture: a Mach-O whose load commands all have cmdsize = 0. +// With ncmds set near INT_MAX the functions hang. The fix breaks out of +// the loop as soon as cmdsize < sizeof(load_command). Found by +// lldb-target-fuzzer. +TEST_F(ObjectFileMachOTest, ZeroCmdSize) { + // Minimal little-endian x86_64 Mach-O: mach_header_64 (32 bytes) followed + // by a single load_command with cmdsize = 0. ncmds is set to 0x7FFFFFFF so + // that without the fix the loops in GetAllArchSpecs/GetUUID never return in + // practice; with the fix they break on the very first iteration. + // Reaching the assertion below is the regression check. + const uint8_t kData[] = { + // mach_header_64 (little-endian) + 0xCF, + 0xFA, + 0xED, + 0xFE, // magic: MH_MAGIC_64 + 0x07, + 0x00, + 0x00, + 0x01, // cputype: CPU_TYPE_X86_64 + 0x03, + 0x00, + 0x00, + 0x80, // cpusubtype: CPU_SUBTYPE_X86_64_ALL + 0x02, + 0x00, + 0x00, + 0x00, // filetype: MH_EXECUTE + 0xFF, + 0xFF, + 0xFF, + 0x7F, // ncmds: 0x7FFFFFFF + 0x08, + 0x00, + 0x00, + 0x00, // sizeofcmds: 8 + 0x00, + 0x00, + 0x00, + 0x00, // flags: 0 + 0x00, + 0x00, + 0x00, + 0x00, // reserved: 0 + // load_command + 0x19, + 0x00, + 0x00, + 0x00, // cmd: LC_SEGMENT_64 (arbitrary) + 0x00, + 0x00, + 0x00, + 0x00, // cmdsize: 0 ← causes the spin + }; + auto Buf = std::make_shared<DataBufferHeap>(kData, sizeof(kData)); + lldb::DataExtractorSP DataSP = std::make_shared<lldb_private::DataExtractor>( + Buf, lldb::eByteOrderLittle, /*addr_size=*/8); + // Before the fix GetAllArchSpecs loops ~0x7FFFFFFF times and never returns. + (void)ObjectFile::GetModuleSpecifications(FileSpec(), DataSP, 0, + sizeof(kData)); +} `````````` </details> https://github.com/llvm/llvm-project/pull/205134 _______________________________________________ lldb-commits mailing list [email protected] https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
