[Lldb-commits] [PATCH] D72751: [LLDB] Add DynamicLoaderWasmDYLD plugin for WebAssembly debugging

[Paolo Severini via Phabricator via lldb-commits]

paolosev added a comment.

> Hi @paolosev, the lldb sanitized bot is flagging a container-overflow error here. I know that this /can/ have FPs when sanitized and unsanitized code is mixed, but we should be in purely sanitized code here, and this looks like a valid report. PTAL.

I think I might have found a problem with the existing code to cache memory read from a remote process. Looking at:

> http://green.lab.llvm.org/green/view/LLDB/job/lldb-cmake-sanitized/992/testReport/junit/lldb-api/functionalities_gdb_remote_client/TestWasm_py/

the error is easily explained. From ProcessGDBRemote::LoadModules we read the initial chunk (512 bytes) of an object file:

  	_lldb.pyd!lldb_private::MemoryCache::Read(unsigned __int64 addr, void * dst, unsigned __int64 dst_len, lldb_private::Status & error) Line 239	C++
  	_lldb.pyd!lldb_private::Process::ReadMemory(unsigned __int64 addr, void * buf, unsigned __int64 size, lldb_private::Status & error) Line 1953	C++
  	_lldb.pyd!lldb_private::Module::GetMemoryObjectFile(const std::shared_ptr<lldb_private::Process> & process_sp, unsigned __int64 header_addr, lldb_private::Status & error, unsigned __int64 size_to_read) Line 300	C++
  	_lldb.pyd!lldb_private::Process::ReadModuleFromMemory(const lldb_private::FileSpec & file_spec, unsigned __int64 header_addr, unsigned __int64 size_to_read) Line 2402	C++
  	_lldb.pyd!lldb_private::DynamicLoader::LoadModuleAtAddress(const lldb_private::FileSpec & file, unsigned __int64 link_map_addr, unsigned __int64 base_addr, bool base_addr_is_offset) Line 212	C++
  	_lldb.pyd!lldb_private::process_gdb_remote::ProcessGDBRemote::LoadModuleAtAddress(const lldb_private::FileSpec & file, unsigned __int64 link_map, unsigned __int64 base_addr, bool value_is_offset) Line 4769	C++
  	_lldb.pyd!lldb_private::process_gdb_remote::ProcessGDBRemote::LoadModules() Line 4803	C++

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:

  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 size 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.

This should be very simple to fix; it's getting a bit late today but I should have a patch early tomorrow. 
Who is the owner of this MemoryCache code I should ask for a review?


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D72751/new/

https://reviews.llvm.org/D72751