llvm / llvm/llvm-project

[LLDB] GetModuleSpecifications mutates shared DataExtractor causing heap corruption

Open
#190,255 3 comments 0 reactions 0 assignees View on GitHub
lldb
Dominant language
LLVM
Stars
40.5k
Forks
18.7k
PR merge metrics
PR metrics pending

Description

`ObjectFileELF::GetModuleSpecifications` receives `extractor_sp` as a non-const reference to a shared `DataExtractorSP`. When the ELF file is larger than the initial read buffer, the function calls `extractor_sp->SetData(data_sp)` to replace the underlying `DataBuffer` with a larger memory-mapped one.

This mutates the shared `DataExtractor` in-place, which has two consequences:

1. Any other `DataExtractor` that shares the same underlying `DataBuffer` (e.g., one stored in an `ObjectFile` instance created from the same extractor) now has stale `m_start`/`m_end` raw pointers into a buffer whose refcount may have dropped to zero. When those objects are later destroyed, freeing them corrupts glibc's malloc free-list metadata, triggering `abort()` with "corrupted double-linked list".

2. Subsequent plugins in the `ObjectFile::GetModuleSpecifications` loop also receive the mutated extractor, which may point to different data than what they expect.

This regression was introduced by #188978 ("Remove data_offset arg from GetModuleSpecifications"), which removed the local `DataExtractor data = *extractor_sp->GetSubsetExtractorSP(data_offset)` copy that previously isolated mutations to a local variable.

**Reproducer:** Load any ELF binary larger than 512 bytes through LLDB on Linux with multiple concurrent readers of the same extractor. The crash manifests as a SIGABRT during debugger teardown after all sessions have closed.

**Fix:** Effectively revert the offending parts in #188978, ie create a local copy of `extractor_sp`'s data and make modifications there:
```
diff --git a/lldb/source/Plugins/ObjectFile/ELF/ObjectFileELF.cpp b/lldb/source/Plugins/ObjectFile/ELF/ObjectFileELF.cpp
index 7cc782cf2823..670e77f1ce43 100644
--- a/lldb/source/Plugins/ObjectFile/ELF/ObjectFileELF.cpp
+++ b/lldb/source/Plugins/ObjectFile/ELF/ObjectFileELF.cpp
@@ -600,9 +600,14 @@ ModuleSpecList ObjectFileELF::GetModuleSpecifications(
return {};
if (ObjectFileELF::MagicBytesMatch(extractor_sp->GetSharedDataBuffer(), 0,
extractor_sp->GetByteSize())) {
+ // Make a local copy to avoid mutating the shared extractor_sp. If the ELF
+ // file is larger than the initial read buffer, the SetData call below would
+ // replace the shared buffer, invalidating any other DataExtractor that
+ // shares it and corrupting the heap during teardown.
+ DataExtractor data(*extractor_sp);
elf::ELFHeader header;
lldb::offset_t header_offset = 0;
- if (header.Parse(*extractor_sp, &header_offset)) {
+ if (header.Parse(data, &header_offset)) {
ModuleSpec spec(file);
// In Android API level 23 and above, bionic dynamic linker is able to
// load .so file directly from zip file. In that case, .so file is
@@ -647,9 +652,9 @@ ModuleSpecList ObjectFileELF::GetModuleSpecifications(
// calculate CRC32 with this data file_offset and
// length. It is important for Android zip .so file, which is a slice
// of a file, to not access the outside of the file slice range.
- if (extractor_sp->GetByteSize() < length)
+ if (data.GetByteSize() < length)
if (DataBufferSP data_sp = MapFileData(file, length, file_offset)) {
- extractor_sp->SetData(data_sp);
+ data.SetData(data_sp);
}
// In case there is header extension in the section #0, the header we
// parsed above could have sentinel values for e_phnum, e_shnum, and
@@ -657,7 +662,7 @@ ModuleSpecList ObjectFileELF::GetModuleSpecifications(
// bigger data source to get the actual values.
if (header.HasHeaderExtension()) {
lldb::offset_t header_offset = 0;
- header.Parse(*extractor_sp, &header_offset);
+ header.Parse(data, &header_offset);
}

uint32_t gnu_debuglink_crc = 0;
@@ -665,7 +670,7 @@ ModuleSpecList ObjectFileELF::GetModuleSpecifications(
SectionHeaderColl section_headers;
lldb_private::UUID &uuid = spec.GetUUID();

- GetSectionHeaderInfo(section_headers, *extractor_sp, header, uuid,
+ GetSectionHeaderInfo(section_headers, data, header, uuid,
gnu_debuglink_file, gnu_debuglink_crc,
spec.GetArchitecture());

@@ -692,12 +697,12 @@ ModuleSpecList ObjectFileELF::GetModuleSpecifications(
// to fallback to something simpler.
if (header.e_type == llvm::ELF::ET_CORE) {
ProgramHeaderColl program_headers;
- GetProgramHeaderInfo(program_headers, *extractor_sp, header);
+ GetProgramHeaderInfo(program_headers, data, header);

core_notes_crc = CalculateELFNotesSegmentsCRC32(program_headers,
- *extractor_sp);
+ data);
} else {
- gnu_debuglink_crc = calc_crc32(0, *extractor_sp);
+ gnu_debuglink_crc = calc_crc32(0, data);
}
}
using u32le = llvm::support::ulittle32_t;

```

Contributor guide

Open the contributing guide

Assessment

This issue has not been assessed yet.

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.