Author: Yao Qi Date: 2026-08-04T08:56:17+01:00 New Revision: 20a2329148626aa66db555558af9d607035a8bf2
URL: https://github.com/llvm/llvm-project/commit/20a2329148626aa66db555558af9d607035a8bf2 DIFF: https://github.com/llvm/llvm-project/commit/20a2329148626aa66db555558af9d607035a8bf2.diff LOG: [lldb][Mach-O] Fix load-command loops spinning on cmdsize = 0 (#205134) 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 Added: Modified: lldb/source/Plugins/ObjectContainer/Mach-O-Fileset/ObjectContainerMachOFileset.cpp lldb/source/Plugins/ObjectFile/Mach-O/ObjectFileMachO.cpp lldb/unittests/ObjectContainer/CMakeLists.txt lldb/unittests/ObjectContainer/ObjectContainerUniversalMachOTest.cpp lldb/unittests/ObjectFile/MachO/TestObjectFileMachO.cpp Removed: ################################################################################ diff --git a/lldb/source/Plugins/ObjectContainer/Mach-O-Fileset/ObjectContainerMachOFileset.cpp b/lldb/source/Plugins/ObjectContainer/Mach-O-Fileset/ObjectContainerMachOFileset.cpp index 706b8e38e9510..bc60c00e64317 100644 --- a/lldb/source/Plugins/ObjectContainer/Mach-O-Fileset/ObjectContainerMachOFileset.cpp +++ b/lldb/source/Plugins/ObjectContainer/Mach-O-Fileset/ObjectContainerMachOFileset.cpp @@ -21,6 +21,31 @@ 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) { + static_assert(offsetof(T, cmd) == 0, "T::cmd must be the first field"); + static_assert(offsetof(T, cmdsize) == sizeof(uint32_t), + "T::cmdsize must immediately follow T::cmd"); + static_assert(std::is_same<decltype(T::cmd), uint32_t>::value, + "T::cmd must be uint32_t"); + static_assert(std::is_same<decltype(T::cmdsize), uint32_t>::value, + "T::cmdsize must be uint32_t"); + 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 +167,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 6e49f211326ff..60b90309bee00 100644 --- a/lldb/source/Plugins/ObjectFile/Mach-O/ObjectFileMachO.cpp +++ b/lldb/source/Plugins/ObjectFile/Mach-O/ObjectFileMachO.cpp @@ -139,6 +139,31 @@ 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) { + static_assert(offsetof(T, cmd) == 0, "T::cmd must be the first field"); + static_assert(offsetof(T, cmdsize) == sizeof(uint32_t), + "T::cmdsize must immediately follow T::cmd"); + static_assert(std::is_same<decltype(T::cmd), uint32_t>::value, + "T::cmd must be uint32_t"); + static_assert(std::is_same<decltype(T::cmdsize), uint32_t>::value, + "T::cmdsize must be uint32_t"); + 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) { @@ -1306,7 +1331,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; @@ -1335,7 +1360,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 @@ -1881,7 +1906,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) @@ -2111,7 +2136,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) { @@ -4452,7 +4477,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) { @@ -4611,7 +4636,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; @@ -4661,7 +4686,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 { @@ -4747,7 +4772,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) { @@ -4895,7 +4920,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) { @@ -5035,7 +5060,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) { @@ -5061,7 +5086,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]; @@ -5111,7 +5136,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'); @@ -5535,7 +5560,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) { @@ -5695,7 +5720,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 50ab4c9557b66..2d4cbacc5169f 100644 --- a/lldb/unittests/ObjectContainer/ObjectContainerUniversalMachOTest.cpp +++ b/lldb/unittests/ObjectContainer/ObjectContainerUniversalMachOTest.cpp @@ -7,6 +7,7 @@ //===----------------------------------------------------------------------===// #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" @@ -14,6 +15,7 @@ #include "lldb/Symbol/ObjectFile.h" #include "lldb/Utility/ArchSpec.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" @@ -120,6 +122,47 @@ 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. + auto ExpectedFile = TestFile::fromYaml(R"( +--- !mach-o +FileHeader: + magic: 0xFEEDFACF + cputype: 0x01000007 + cpusubtype: 0x80000003 + filetype: 0x0000000C + ncmds: 0x7FFFFFFF + sizeofcmds: 8 + flags: 0x00000000 + reserved: 0x00000000 +LoadCommands: + - cmd: LC_THREAD + cmdsize: 0 +... +)"); + ASSERT_THAT_EXPECTED(ExpectedFile, llvm::Succeeded()); + + ModuleSpec Spec = ExpectedFile->moduleSpec(); + lldb::DataExtractorSP DataSP = Spec.GetExtractor(); + // Before the fix ParseFileset loops ~0x7FFFFFFF times and never returns. + (void)ObjectContainerMachOFileset::GetModuleSpecifications( + FileSpec(), DataSP, 0, DataSP->GetByteSize()); +} + // Regression fixture: a universal (fat) Mach-O whose header claims a huge // nfat_arch (here 0xAFAFAFAF) but provides no fat_arch entries beyond the // header bytes. Found by lldb-target-fuzzer. diff --git a/lldb/unittests/ObjectFile/MachO/TestObjectFileMachO.cpp b/lldb/unittests/ObjectFile/MachO/TestObjectFileMachO.cpp index b3a238022aa57..7ebbcd6d1974d 100644 --- a/lldb/unittests/ObjectFile/MachO/TestObjectFileMachO.cpp +++ b/lldb/unittests/ObjectFile/MachO/TestObjectFileMachO.cpp @@ -17,6 +17,7 @@ #include "lldb/Host/HostInfo.h" #include "lldb/Symbol/ObjectFile.h" #include "lldb/Symbol/Symtab.h" +#include "lldb/Utility/DataExtractor.h" #include "lldb/Utility/FileSpec.h" #include "lldb/lldb-defines.h" #include "llvm/Testing/Support/Error.h" @@ -112,6 +113,41 @@ TEST_F(ObjectFileMachOTest, IndirectSymbolsInTheSharedCache) { } #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. + auto ExpectedFile = TestFile::fromYaml(R"( +--- !mach-o +FileHeader: + magic: 0xFEEDFACF + cputype: 0x01000007 + cpusubtype: 0x80000003 + filetype: 0x00000002 + ncmds: 0x7FFFFFFF + sizeofcmds: 8 + flags: 0x00000000 + reserved: 0x00000000 +LoadCommands: + - cmd: LC_THREAD + cmdsize: 0 +... +)"); + ASSERT_THAT_EXPECTED(ExpectedFile, llvm::Succeeded()); + + ModuleSpec Spec = ExpectedFile->moduleSpec(); + lldb::DataExtractorSP DataSP = Spec.GetExtractor(); + // Before the fix GetAllArchSpecs loops ~0x7FFFFFFF times and never returns. + (void)ObjectFile::GetModuleSpecifications(FileSpec(), DataSP, 0, + DataSP->GetByteSize()); +} + // A Mach-O whose MH_DYLIB_IN_CACHE flag is set but which has no __LINKEDIT // segment. TEST_F(ObjectFileMachOTest, ParseSymtabSharedCacheMissingLinkedit) { _______________________________________________ lldb-commits mailing list [email protected] https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
