[LLDB] AddressSanitizer failure in MemoryCache

[paolosev]

Summary

The original discussion for this issues is here.

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.

[clayborg]

Not sure this is the right fix. The calling code should listen to how many bytes were actually read from memory and avoid touching any bytes. So we should fix Module::GetMemoryObjectFile to resize its buffer back to only the size that was read. Something like:

const size_t bytes_read =
    process_sp->ReadMemory(header_addr, data_up->GetBytes(),
                           data_up->GetByteSize(), readmem_error);
if (bytes_read < size_to_read)
  data_sp->SetByteSize(bytes_read);
if (data_sp->GetByteSize() > 0) {
  DataBufferSP data_sp(data_up.release());
  m_objfile_sp = ObjectFile::FindPlugin(shared_from_this(), process_sp,
                                        header_addr, data_sp);

[paolosev]

In D75200#1894162, @clayborg wrote:

Not sure this is the right fix. The calling code should listen to how many bytes were actually read from memory and avoid touching any bytes. So we should fix Module::GetMemoryObjectFile to resize its buffer back to only the size that was read. S

I agree that the previous fix wasn't great, but it had the advantage that it did not change the semantics of MemoryCache::Read. I was afraid there could be other places where the caller might assume that MemoryCache::Read fills the whole buffer even when it does not have enough data to copy, even though it was a strange semantics.

All the 'check-lldb' tests still pass though.

[paolosev]

The Harbormaster errors don't seem to be real failures:

/mnt/disks/ssd0/agent/workspace/amd64_debian_testing_clang8/lldb/source/Core/Module.cpp:9:10: error: 'lldb/Core/Module.h' file not found [clang-diagnostic-error]
#include "lldb/Core/Module.h"
         ^
/mnt/disks/ssd0/agent/workspace/amd64_debian_testing_clang8/lldb/source/Target/Memory.cpp:9:10: error: 'lldb/Target/Memory.h' file not found [clang-diagnostic-error]
#include "lldb/Target/Memory.h"
         ^

[teemperor]

This fixes the asan failures for me, so LGTM. Don't really know that code so someone else should approve this. I skipped the test on the sanitizer build for now ( 56b03c35ddecf7d096a513b0633ddf6c49286784 ).

[labath]

Yea, I guess I spoke too soon when I said the fix is going to be easy -- this is quite messy.

I believe I understand now why we haven't run into this problem before . Lldb default cache size is 1024 bytes, and the typical page size is 4k. This means a single read will always stay within the confines of one memory page, and so we're very unlikely to get a partial read.

In this sense the wasm test is a bit atypical, because normally we would just end up reading random parts of inferior memory in case of tiny object files. Nevertheless, I don't think this code should assume any particular page size, and so I think truncating the buffer is the right thing to do. Judging by the comment on line 214, this code already expects this situation (although I wouldn't be surprised if there were other bugs in handling incomplete reads lurking around here).

lldb/source/Core/Module.cpp

if (m_objfile_sp) {
     std::lock_guard<std::recursive_mutex> guard(m_mutex);
     if (process_sp) {
       m_did_load_objfile = true;
       auto data_up = std::make_unique<DataBufferHeap>(size_to_read, 0);
       Status readmem_error;
       const size_t bytes_read =
           process_sp->ReadMemory(header_addr, data_up->GetBytes(),
                                  data_up->GetByteSize(), readmem_error);
-      if (bytes_read == size_to_read) {
+      if (bytes_read < size_to_read)
+        data_up->SetByteSize(bytes_read);
+      if (data_up->GetByteSize() > 0) {
         DataBufferSP data_sp(data_up.release());
         m_objfile_sp = ObjectFile::FindPlugin(shared_from_this(), process_sp,
                                               header_addr, data_sp);
         if (m_objfile_sp) {
           StreamString s;
           s.Printf("0x%16.16" PRIx64, header_addr);
           m_object_name.SetString(s.GetString());
 

lldb/source/Target/Memory.cpp

while (bytes_left > 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)
+        if (process_bytes_read != cache_line_byte_size) {
+          if (process_bytes_read < data_buffer_heap_up->GetByteSize()) {
+            dst_len -= data_buffer_heap_up->GetByteSize() - process_bytes_read;
+            bytes_left = process_bytes_read;
+          }
           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...
       }
     }
   }
 
   return dst_len - bytes_left;