paolosev created this revision.
paolosev added reviewers: labath, vsk, JDevlieghere, clayborg.
paolosev added a project: LLDB.
Herald added subscribers: lldb-commits, sunfish, aheejin.
The original discussion for this issues is here
<https://reviews.llvm.org/D72751#1892647>.
The lldb sanitized bot is flagging a container-overflow error after we
introduced test `TestWasm.py`:
http://green.lab.llvm.org/green/view/LLDB/job/lldb-cmake-sanitized/992/testReport/junit/lldb-api/functionalities_gdb_remote_client/TestWasm_py/
11283==ERROR: AddressSanitizer: container-overflow on address 0x615000016184 at
pc 0x00010b4608f0 bp 0x7ffee4f00130 sp 0x7ffee4eff8f8
-------------------------------------------------------------------------------------------------------------------------------------
READ of size 512 at 0x615000016184 thread T0
#0 0x10b4608ef in __asan_memcpy+0x1af
(libclang_rt.asan_osx_dynamic.dylib:x86_64+0x418ef)
#1 0x1116486d5 in lldb_private::MemoryCache::Read(unsigned long long, void*,
unsigned long, lldb_private::Status&) Memory.cpp:189
#2 0x11119d0e9 in
lldb_private::Module::GetMemoryObjectFile(std::__1::shared_ptr<lldb_private::Process>
const&, unsigned long long, lldb_private::Status&, unsigned long)
Module.cpp:298
#3 0x11169eeef in
lldb_private::Process::ReadModuleFromMemory(lldb_private::FileSpec const&,
unsigned long long, unsigned long) Process.cpp:2402
#4 0x11113337b in
lldb_private::DynamicLoader::LoadModuleAtAddress(lldb_private::FileSpec const&,
unsigned long long, unsigned long long, bool) DynamicLoader.cpp:212
#5 0x111ed53da in
lldb_private::process_gdb_remote::ProcessGDBRemote::LoadModuleAtAddress(lldb_private::FileSpec
const&, unsigned long long, unsigned long long, bool) ProcessGDBRemote.cpp:4767
#6 0x111ed59b8 in
lldb_private::process_gdb_remote::ProcessGDBRemote::LoadModules()
ProcessGDBRemote.cpp:4801
#7 0x1119c59aa in lldb_private::wasm::DynamicLoaderWasmDYLD::DidAttach()
DynamicLoaderWasmDYLD.cpp:63
#8 0x1116a3a97 in lldb_private::Process::CompleteAttach() Process.cpp:2930
#9 0x1116a6bdf in lldb_private::Process::ConnectRemote(lldb_private::Stream*,
llvm::StringRef) Process.cpp:3015
#10 0x110a362ee in lldb::SBTarget::ConnectRemote(lldb::SBListener&, char
const*, char const*, lldb::SBError&) SBTarget.cpp:559
There is actually a problem in the MemoryCache code. From
ProcessGDBRemote::LoadModules we read the initial chunk (512 bytes) of an
object file. In MemoryCache::Read, since this data is not cached yet, we call
m_process.ReadMemoryFromInferior to actually read the memory (lines 174-241),
look at the bottom of:
size_t MemoryCache::Read(addr_t addr, void *dst, size_t dst_len,
Status &error) {
[...]
while (bytes_left > 0) {
[...]
BlockMap::const_iterator pos = m_L2_cache.find(curr_addr);
BlockMap::const_iterator end = m_L2_cache.end();
if (pos != end) {
size_t curr_read_size = cache_line_byte_size - cache_offset;
if (curr_read_size > bytes_left)
curr_read_size = bytes_left;
memcpy(dst_buf + dst_len - bytes_left,
pos->second->GetBytes() + cache_offset, curr_read_size);
[...]
}
// We need to read from the process
if (bytes_left > 0) {
assert((curr_addr % cache_line_byte_size) == 0);
std::unique_ptr<DataBufferHeap> data_buffer_heap_up(
new DataBufferHeap(cache_line_byte_size, 0));
size_t process_bytes_read = m_process.ReadMemoryFromInferior(
curr_addr, data_buffer_heap_up->GetBytes(),
data_buffer_heap_up->GetByteSize(), error);
if (process_bytes_read == 0)
return dst_len - bytes_left;
if (process_bytes_read != cache_line_byte_size)
data_buffer_heap_up->SetByteSize(process_bytes_read);
m_L2_cache[curr_addr] = DataBufferSP(data_buffer_heap_up.release());
// We have read data and put it into the cache, continue through the
// loop again to get the data out of the cache...
}
First, we allocate a DataBufferHeap with the size of our cache_line_byte_size,
512 bytes, and we pass it to ReadMemoryFromInferior().
The problem is that in this test the whole object file is only 0x84 bytes, so
we resize data_buffer_heap_up to a smaller size with
data_buffer_heap_up->SetByteSize(process_bytes_read).
Then we iterate back up in the while loop, and try to read from this
reallocated buffer. But we still try to read curr_read_size==512 bytes, so read
past the buffer size. In fact the overflow is at address 0x615000016184 for a
buffer that starts at 0x615000016100.
Note that the simple fix of just reading the available bytes doesn't work:
Module::GetMemoryObjectFile expects to always be able to read at least 512
bytes, and it fails if the object file is smaller:
ObjectFile *Module::GetMemoryObjectFile(const lldb::ProcessSP &process_sp,
lldb::addr_t header_addr, Status
&error,
size_t size_to_read) {
[...]
const size_t bytes_read =
process_sp->ReadMemory(header_addr, data_up->GetBytes(),
data_up->GetByteSize(), readmem_error);
if (bytes_read == size_to_read) {
[...] // ok...
} else {
error.SetErrorStringWithFormat("unable to read header from memory:
%s",
readmem_error.AsCString());
}
[...]
To fix the issue, this patch avoids to resize the DataBufferHeap to a smaller
size and pads it with a filler (0xdd) if necessary.
Repository:
rG LLVM Github Monorepo
https://reviews.llvm.org/D75200
Files:
lldb/source/Target/Memory.cpp
Index: lldb/source/Target/Memory.cpp
===================================================================
--- lldb/source/Target/Memory.cpp
+++ lldb/source/Target/Memory.cpp
@@ -232,8 +232,17 @@
if (process_bytes_read == 0)
return dst_len - bytes_left;
- if (process_bytes_read != cache_line_byte_size)
- data_buffer_heap_up->SetByteSize(process_bytes_read);
+ if (process_bytes_read != cache_line_byte_size) {
+ // If the data available to read is less than an entire page, we
don't
+ // resize the buffer because we still want to return at least
+ // 'dst_len' bytes. In that case fill the rest of the buffer with a
+ // fail-fill value (0xdd).
+ if (process_bytes_read >= data_buffer_heap_up->GetByteSize())
+ data_buffer_heap_up->SetByteSize(process_bytes_read);
+ else
+ memset(data_buffer_heap_up->GetBytes() + process_bytes_read, 0xdd,
+ data_buffer_heap_up->GetByteSize() - process_bytes_read);
+ }
m_L2_cache[curr_addr] = DataBufferSP(data_buffer_heap_up.release());
// We have read data and put it into the cache, continue through the
// loop again to get the data out of the cache...
Index: lldb/source/Target/Memory.cpp
===================================================================
--- lldb/source/Target/Memory.cpp
+++ lldb/source/Target/Memory.cpp
@@ -232,8 +232,17 @@
if (process_bytes_read == 0)
return dst_len - bytes_left;
- if (process_bytes_read != cache_line_byte_size)
- data_buffer_heap_up->SetByteSize(process_bytes_read);
+ if (process_bytes_read != cache_line_byte_size) {
+ // If the data available to read is less than an entire page, we don't
+ // resize the buffer because we still want to return at least
+ // 'dst_len' bytes. In that case fill the rest of the buffer with a
+ // fail-fill value (0xdd).
+ if (process_bytes_read >= data_buffer_heap_up->GetByteSize())
+ data_buffer_heap_up->SetByteSize(process_bytes_read);
+ else
+ memset(data_buffer_heap_up->GetBytes() + process_bytes_read, 0xdd,
+ data_buffer_heap_up->GetByteSize() - process_bytes_read);
+ }
m_L2_cache[curr_addr] = DataBufferSP(data_buffer_heap_up.release());
// We have read data and put it into the cache, continue through the
// loop again to get the data out of the cache...
_______________________________________________
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
