This is an archive of the discontinued LLVM Phabricator instance.

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

Authored by mstorsjo on Oct 28 2019, 4:23 AM.

Download Raw Diff

Details

Reviewers

labath
amccarth
aleksandr.urakov

Commits

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

Summary

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

Diff Detail

Repository: rG LLVM Github Monorepo

Event Timeline

mstorsjo created this revision.Oct 28 2019, 4:23 AM

Herald added a project: Restricted Project. · View Herald TranscriptOct 28 2019, 4:23 AM

Herald added subscribers: lldb-commits, JDevlieghere. · View Herald Transcript

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

This revision is now accepted and ready to land.Oct 28 2019, 7:27 AM

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

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

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?

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

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.

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

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.

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

labath accepted this revision.Oct 31 2019, 1:26 AM

labath added inline comments.

lldb/test/Shell/Minidump/Windows/Inputs/broken-unwind.exe.yaml
30	Maybe add a comment that this is the thing which points outside the .pdata (or any other) section.

Closed by commit rG7e1a3076419d: [LLDB] [PECOFF] Don't crash in ReadImageDataByRVA for addresses out of range (authored by mstorsjo). · Explain WhyOct 31 2019, 2:30 AM

This revision was automatically updated to reflect the committed changes.

Revision Contents

Path

Size

lldb/

source/

Plugins/

ObjectFile/

PECOFF/

ObjectFilePECOFF.cpp

5 lines

test/

Shell/

Minidump/

Windows/

Inputs/

broken-unwind.dmp.yaml

35 lines

broken-unwind.exe.yaml

87 lines

broken-unwind.test

7 lines

Diff 227246

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

Show First 20 Lines • Show All 558 Lines • ▼ Show 20 Lines	if (process_sp) {
}		}
}		}
return data;		return data;
}		}

DataExtractor ObjectFilePECOFF::ReadImageDataByRVA(uint32_t rva, size_t size) {		DataExtractor ObjectFilePECOFF::ReadImageDataByRVA(uint32_t rva, size_t size) {
if (m_file) {		if (m_file) {
Address addr = GetAddress(rva);		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);		return ReadImageData(rva, size);
}		}

// ParseSectionHeaders		// ParseSectionHeaders
bool ObjectFilePECOFF::ParseSectionHeaders(		bool ObjectFilePECOFF::ParseSectionHeaders(
uint32_t section_header_data_offset) {		uint32_t section_header_data_offset) {
▲ Show 20 Lines • Show All 706 Lines • Show Last 20 Lines

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

This file was added.

				--- !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

This file was added.

				# The ExceptionTable RelativeVirtualAddress below points outside of allocated
				# sections.

				--- !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
				labathUnsubmitted Not Done Reply Inline Actions Maybe add a comment that this is the thing which points outside the .pdata (or any other) section. labath: Maybe add a comment that this is the thing which points outside the .pdata (or any other)…
				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

This file was added.

				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