[LLDB] [PECOFF] Don't crash in ReadImageDataByRVA for addresses out of range

[mstorsjo]

Summary

This can happen e.g. when unwinding doesn't work perfectly.

[aleksandr.urakov]

LGTM, thank you! Can you send me an example of binary on which unwinding fails with a crash, please? It is a very interesting case...

[mstorsjo]

In D69502#1723385, @aleksandr.urakov wrote:

LGTM, thank you! Can you send me an example of binary on which unwinding fails with a crash, please? It is a very interesting case...

It is on arm64, so it's not something that is expected to be flawless yet (even if it generally works, sometimes).

[labath]

Any way to get a test for this? Maybe creating a object file with a bogus unwind RVA via yaml2obj ?

[mstorsjo]

In D69502#1723549, @labath wrote:

Any way to get a test for this? Maybe creating a object file with a bogus unwind RVA via yaml2obj ?

Do we have a suitable test as basis for it? I'm not quite sure which way is the most compact way of achieving that. A small couple function exe with SEH or dwarf (eh_frame) unwind info, without debug info, with a crash/int3 in a nested function? Or just some image unwind commands so it doesn't need executing?

[labath]

In D69502#1723715, @mstorsjo wrote:

In D69502#1723549, @labath wrote:

Any way to get a test for this? Maybe creating a object file with a bogus unwind RVA via yaml2obj ?

Do we have a suitable test as basis for it? I'm not quite sure which way is the most compact way of achieving that. A small couple function exe with SEH or dwarf (eh_frame) unwind info, without debug info, with a crash/int3 in a nested function? Or just some image unwind commands so it doesn't need executing?

We have some files that might be usable as a basis for this, but I don't know which one would be the best, as I don't know what you need here. What do you need to do in order to reproduce the crash? Would it be possible to just set the export table RVA to some bogus value? That should be trigerred by just constructing the module symbol table...

[mstorsjo]

In D69502#1725146, @labath wrote:

In D69502#1723715, @mstorsjo wrote:

In D69502#1723549, @labath wrote:

Any way to get a test for this? Maybe creating a object file with a bogus unwind RVA via yaml2obj ?

Do we have a suitable test as basis for it? I'm not quite sure which way is the most compact way of achieving that. A small couple function exe with SEH or dwarf (eh_frame) unwind info, without debug info, with a crash/int3 in a nested function? Or just some image unwind commands so it doesn't need executing?

We have some files that might be usable as a basis for this, but I don't know which one would be the best, as I don't know what you need here. What do you need to do in order to reproduce the crash? Would it be possible to just set the export table RVA to some bogus value? That should be trigerred by just constructing the module symbol table...

Ok, I'll look at it later to see if I can make some broken file to trigger this condition.

[mstorsjo]

In D69502#1725155, @mstorsjo wrote:

In D69502#1725146, @labath wrote:

In D69502#1723715, @mstorsjo wrote:

In D69502#1723549, @labath wrote:

Any way to get a test for this? Maybe creating a object file with a bogus unwind RVA via yaml2obj ?

Do we have a suitable test as basis for it? I'm not quite sure which way is the most compact way of achieving that. A small couple function exe with SEH or dwarf (eh_frame) unwind info, without debug info, with a crash/int3 in a nested function? Or just some image unwind commands so it doesn't need executing?

We have some files that might be usable as a basis for this, but I don't know which one would be the best, as I don't know what you need here. What do you need to do in order to reproduce the crash? Would it be possible to just set the export table RVA to some bogus value? That should be trigerred by just constructing the module symbol table...

Ok, I'll look at it later to see if I can make some broken file to trigger this condition.

I gave this some amount of tries, but my files with broken unwind info doesn't trigger it. It happened originally unreliably on arm64.

Ok to proceed with it without a testcase?

I did test crafting a file with a bogus export table RVA as well, and that crashes lldb elsewhere, due to an unchecked Expected<> which contained an error. Will try to look into that one separately later...

[labath]

Try this for patch for size:

a.patch9 KBDownload

. It needs to me moved into a separate test case, and cleaned up a bit, but I believe it should trigger the bug you're fixing here.

The tricky part about unwind info is that you currently need a Process instance around even to just inspect (= parse) it. I have ideas how to fix that, but no time to implement them :(. Until then we need to create a process somehow to test unwind info parsing.

Fortunately, these days we have a minidump files and their yaml representation, which gives us a relatively easy (though there definitely are ways to improve that too) method to create Processes from core files. I consider something like this to be the future of testing unwinding, because it's usually very hard to reliably drive a real process to a point which will exercise some specific unwind corner case. OTOH, with a minidump(core) file, you can just freeze a process in the state that you're intersted in, and the test will always test the exact same thing.

[mstorsjo]

Added a testcase based on @labath 's patch. (Thanks! That managed to trigger the condition!)

[labath]

lldb/test/Shell/Minidump/Windows/Inputs/broken-unwind.exe.yaml

29

Maybe add a comment that this is the thing which points outside the .pdata (or any other) section.

lldb/source/Plugins/ObjectFile/PECOFF/ObjectFilePECOFF.cpp

if (process_sp) {
     }
   }
   return data;
 }
 
 DataExtractor ObjectFilePECOFF::ReadImageDataByRVA(uint32_t rva, size_t size) {
   if (m_file) {
     Address addr = GetAddress(rva);
-    rva = addr.GetSection()->GetFileOffset() + addr.GetOffset();
+    SectionSP sect = addr.GetSection();
+    if (!sect)
+      return {};
+    rva = sect->GetFileOffset() + addr.GetOffset();
   }
 
   return ReadImageData(rva, size);
 }
 
 // ParseSectionHeaders
 bool ObjectFilePECOFF::ParseSectionHeaders(
     uint32_t section_header_data_offset) {

lldb/test/Shell/Minidump/Windows/Inputs/broken-unwind.dmp.yaml

+--- !minidump
+Streams:
+  - Type:            ModuleList
+    Modules:
+      - Base of Image:   0x00000000000B0000
+        Size of Image:   0x00005000
+        Module Name:     'find-module.exe'
+        CodeView Record: 52534453E092B2141AD8F1B44C4C44205044422E01000000433A5C70726F6A656374735C746573745F6170705C436F6E736F6C654170706C69636174696F6E315C44656275675C436F6E736F6C654170706C69636174696F6E312E70646200
+  - Type:            SystemInfo
+    Processor Arch:  X86
+    Processor Level: 23
+    Processor Revision: 2050
+    Number of Processors: 32
+    Product type:    1
+    Major Version:   10
+    Build Number:    17134
+    Platform ID:     Win32NT
+    Suite Mask:      0x0300
+    CPU:
+      Vendor ID:       AuthenticAMD
+      Version Info:    0x00800F82
+      Feature Info:    0x178BFBFF
+      AMD Extended Features: 0x2FD3FBFF
+  - Type:            MiscInfo
+    Content:         54050000F703000008290000C883495D0000000000000000AC0D000098080000AC0D00000200000002000000002000000D0000000000000002000000C4FFFFFF430065006E007400720061006C0020004500750072006F007000650020005300740061006E0064006100720064002000540069006D006500000000000000000000000A0000000500030000000000000000000000430065006E007400720061006C0020004500750072006F00700065002000530075006D006D00650072002000540069006D00650000000000000000000000000000000300000005000200000000000000C4FFFFFF310037003100330034002E0031002E007800380036006600720065002E007200730034005F00720065006C0065006100730065002E003100380030003400310030002D00310038003000340000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000064006200670063006F00720065002E0069003300380036002C00310030002E0030002E00310037003100330034002E0031000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000006C816B92
+  - Type:            ThreadList
+    Threads:
+      - Thread Id:       0x0000290C
+        Priority Class:  0x00000020
+        Environment Block: 0x0000000000A98000
+        Context:         3F0001000000000000000000000000000000000000000000000000007F02000000000000FFFF0000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000002B000000530000002B0000002B00000080100B0080100B000050A90080100B0080100B0000000000E4FECF0092100B0023000000440301007CFECF002B0000007F0200000000000000000000000000000000000000000000801F0000FFFF020000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000144E3D6C2000000001000000EA1E00F4C000E50700000000FC0100002CE3C014D8E202010000E507880F401D839DC60100000000400000007F00000000000000880F401D0A000000900F401D0000000000000100EA1E00009808E5077F00000000000000000000009008E507990100006002E5072CABC87708346474B42300000100000044E3C014200000000000000020532777A80F401D4F346474D00F401D6F378CCC5C4CD5010000000000000000000000000000000000000000000000003AFCD72F90E3C01418CE3470B4230000B80F401DC00F401DC80F401DD00F401D
+        Stack:
+          Start of Memory Range: 0x0000000000CFFE78
+          Content:         0000000079100B0000100B0000100B0000100B0000100B0000100B0000100B0000100B0000100B0000100B0000100B0000100B0000100B0000100B0000100B0000100B0000100B0000100B0000100B0000100B0000100B0085100B0094842777
+...

lldb/test/Shell/Minidump/Windows/Inputs/broken-unwind.exe.yaml

+--- !COFF
+OptionalHeader:
+  AddressOfEntryPoint: 4224
+  ImageBase:       4194304
+  SectionAlignment: 4096
+  FileAlignment:   512
+  MajorOperatingSystemVersion: 6
+  MinorOperatingSystemVersion: 0
+  MajorImageVersion: 0
+  MinorImageVersion: 0
+  MajorSubsystemVersion: 6
+  MinorSubsystemVersion: 0
+  Subsystem:       IMAGE_SUBSYSTEM_WINDOWS_CUI
+  DLLCharacteristics: [ IMAGE_DLL_CHARACTERISTICS_DYNAMIC_BASE, IMAGE_DLL_CHARACTERISTICS_NX_COMPAT, IMAGE_DLL_CHARACTERISTICS_NO_SEH, IMAGE_DLL_CHARACTERISTICS_TERMINAL_SERVER_AWARE ]
+  SizeOfStackReserve: 1048576
+  SizeOfStackCommit: 4096
+  SizeOfHeapReserve: 1048576
+  SizeOfHeapCommit: 4096
+  ExportTable:     
+    RelativeVirtualAddress: 8327
+    Size:            90
+  ImportTable:     
+    RelativeVirtualAddress: 0
+    Size:            0
+  ResourceTable:   
+    RelativeVirtualAddress: 0
+    Size:            0
+  ExceptionTable:  
+    RelativeVirtualAddress: 12303
+    Size:            12
+  CertificateTable: 
+    RelativeVirtualAddress: 0
+    Size:            0
+  BaseRelocationTable: 
+    RelativeVirtualAddress: 0
+    Size:            0
+  Debug:           
+    RelativeVirtualAddress: 8192
+    Size:            28
+  Architecture:    
+    RelativeVirtualAddress: 0
+    Size:            0
+  GlobalPtr:       
+    RelativeVirtualAddress: 0
+    Size:            0
+  TlsTable:        
+    RelativeVirtualAddress: 0
+    Size:            0
+  LoadConfigTable: 
+    RelativeVirtualAddress: 0
+    Size:            0
+  BoundImport:     
+    RelativeVirtualAddress: 0
+    Size:            0
+  IAT:             
+    RelativeVirtualAddress: 0
+    Size:            0
+  DelayImportDescriptor: 
+    RelativeVirtualAddress: 0
+    Size:            0
+  ClrRuntimeHeader: 
+    RelativeVirtualAddress: 0
+    Size:            0
+header:
+  Machine:         IMAGE_FILE_MACHINE_I386
+  Characteristics: [ IMAGE_FILE_EXECUTABLE_IMAGE, IMAGE_FILE_32BIT_MACHINE ]
+sections:
+  - Name:            .text
+    Characteristics: [ IMAGE_SCN_CNT_CODE, IMAGE_SCN_MEM_EXECUTE, IMAGE_SCN_MEM_READ ]
+    VirtualAddress:  4096
+    VirtualSize:     22
+    SectionData:     50894C24048B4C24040FAF4C2404890C248B042459C3
+  - Name:            .rdata
+    Characteristics: [ IMAGE_SCN_CNT_INITIALIZED_DATA, IMAGE_SCN_MEM_READ ]
+    VirtualAddress:  8192
+    VirtualSize:     236
+    SectionData:     00000000A565B65C00000000020000006B0000001C2000001C06000052534453E092B2141AD8F1B44C4C44205044422E01000000443A5C757073747265616D5C6275696C645C746F6F6C735C6C6C64625C6C69745C4D6F64756C65735C5045434F46465C4F75747075745C6578706F72742D646C6C66756E632E79616D6C2E746D702E70646200000000000000000000000000AF200000000000000200000001000000CB200000D3200000D72000006578706F72742D646C6C66756E632E79616D6C2E746D702E646C6C000000000000100000D92000000100446C6C46756E63000000000101010001020000
+  - Name:            .pdata
+    Characteristics: [ IMAGE_SCN_CNT_INITIALIZED_DATA, IMAGE_SCN_MEM_READ ]
+    VirtualAddress:  12288
+    VirtualSize:     12
+    SectionData:     '0010000016100000E4200000'
+symbols:         []
+...

lldb/test/Shell/Minidump/Windows/broken-unwind.test

+Test that we can cope with broken unwind information that suggests
+reading out of bounds.
+
+RUN: yaml2obj %S/Inputs/broken-unwind.exe.yaml > %T/broken-unwind.exe
+RUN: yaml2obj %S/Inputs/broken-unwind.dmp.yaml > %T/broken-unwind.dmp
+RUN: %lldb -O "settings set target.exec-search-paths %T" \
+RUN:   -c %T/broken-unwind.dmp -o "image show-unwind -a 0xb1000" -o exit