From 14135f50a036af4d3a64b8e2e0dc2ecda5260533 Mon Sep 17 00:00:00 2001 From: Vedant Kumar Date: Tue, 21 Jan 2020 16:01:16 -0800 Subject: [PATCH] [lldb/Value] Avoid reading more data than the host has available Value::GetValueByteSize() reports the size of a Value as the size of its underlying CompilerType. However, a host buffer that backs a Value may be smaller than GetValueByteSize(). This situation arises when the host is only able to partially evaluate a Value, e.g. because the expression contains DW_OP_piece. The cleanest fix I've found to this problem is Greg's suggestion, which is to resize the Value if (after evaluating an expression) it's found to be too small. I've tried several alternatives which all (in one way or the other) tried to teach the Value/ValueObjectChild system not to read past the end of a host buffer, but this was flaky and impractical as it isn't easy to figure out the host buffer's size (Value::GetScalar() can point to somewhere /inside/ a host buffer, but you need to walk up the ValueObject hierarchy to try and find its size). This fixes an ASan error in lldb seen when debugging a clang binary. I've added a regression test in test/functionalities/optimized_code. The point of that test is not specifically to check that DW_OP_piece is handled a particular way, but rather to check that lldb doesn't crash on an input that it used to crash on. Testing: check-lldb, and running the added tests using a sanitized lldb -- Thanks to Jim for pointing out that an earlier version of this patch, which simply changed the definition of Value::GetValueByteSize(), would interact poorly with the ValueObject machinery. Thanks also to Pavel who suggested a neat way to test this change (which, incidentally, caught another ASan issue still present in the original version of this patch). rdar://58665925 Differential Revision: https://reviews.llvm.org/D73148 --- .../test/functionalities/optimized_code/Makefile | 3 + .../TestNoASanExceptionAfterEvalOP_piece.py | 4 + .../test/functionalities/optimized_code/main.cpp | 31 ++++++ lldb/source/Core/ValueObjectVariable.cpp | 21 ++++ .../DWARF/DW_OP_piece-smaller-than-struct.s | 110 +++++++++++++++++++++ 5 files changed, 169 insertions(+) create mode 100644 lldb/packages/Python/lldbsuite/test/functionalities/optimized_code/Makefile create mode 100644 lldb/packages/Python/lldbsuite/test/functionalities/optimized_code/TestNoASanExceptionAfterEvalOP_piece.py create mode 100644 lldb/packages/Python/lldbsuite/test/functionalities/optimized_code/main.cpp create mode 100644 lldb/test/Shell/SymbolFile/DWARF/DW_OP_piece-smaller-than-struct.s diff --git a/lldb/packages/Python/lldbsuite/test/functionalities/optimized_code/Makefile b/lldb/packages/Python/lldbsuite/test/functionalities/optimized_code/Makefile new file mode 100644 index 00000000000..129330fd9fa --- /dev/null +++ b/lldb/packages/Python/lldbsuite/test/functionalities/optimized_code/Makefile @@ -0,0 +1,3 @@ +CXX_SOURCES := main.cpp +CXXFLAGS_EXTRAS := -O2 +include Makefile.rules diff --git a/lldb/packages/Python/lldbsuite/test/functionalities/optimized_code/TestNoASanExceptionAfterEvalOP_piece.py b/lldb/packages/Python/lldbsuite/test/functionalities/optimized_code/TestNoASanExceptionAfterEvalOP_piece.py new file mode 100644 index 00000000000..c8308c16011 --- /dev/null +++ b/lldb/packages/Python/lldbsuite/test/functionalities/optimized_code/TestNoASanExceptionAfterEvalOP_piece.py @@ -0,0 +1,4 @@ +from lldbsuite.test import lldbinline +from lldbsuite.test import decorators + +lldbinline.MakeInlineTest(__file__, globals()) diff --git a/lldb/packages/Python/lldbsuite/test/functionalities/optimized_code/main.cpp b/lldb/packages/Python/lldbsuite/test/functionalities/optimized_code/main.cpp new file mode 100644 index 00000000000..f1beaf3020d --- /dev/null +++ b/lldb/packages/Python/lldbsuite/test/functionalities/optimized_code/main.cpp @@ -0,0 +1,31 @@ +// This is a regression test that checks whether lldb can inspect the variables +// in this program without triggering an ASan exception. + +__attribute__((noinline, optnone)) int use(int x) { return x; } + +volatile int sink; + +struct S1 { + int f1; + int *f2; +}; + +struct S2 { + char a, b; + int pad; + S2(int x) { + a = x & 0xff; + b = x & 0xff00; + } +}; + +int main() { + S1 v1; + v1.f1 = sink; + v1.f2 = nullptr; + sink++; //% self.expect("frame variable v1", substrs=["S1"]) + S2 v2(v1.f1); + sink += use(v2.a); //% self.expect("frame variable v2", substrs=["S2"]) + sink += use(v2.pad); //% self.expect("frame variable v2", substrs=["S2"]) + return 0; +} diff --git a/lldb/source/Core/ValueObjectVariable.cpp b/lldb/source/Core/ValueObjectVariable.cpp index 945e5c411ec..d0664276dc1 100644 --- a/lldb/source/Core/ValueObjectVariable.cpp +++ b/lldb/source/Core/ValueObjectVariable.cpp @@ -166,6 +166,27 @@ bool ValueObjectVariable::UpdateValue() { Value::ValueType value_type = m_value.GetValueType(); + // The size of the buffer within m_value can be less than the size + // prescribed by its type. E.g. this can happen when an expression only + // partially describes an object (say, because it contains DW_OP_piece). + // + // In this case, grow m_value to the expected size. An alternative way to + // handle this is to teach Value::GetValueAsData() and ValueObjectChild + // not to read past the end of a host buffer, but this gets impractically + // complicated as a Value's host buffer may be shared with a distant + // ancestor or sibling in the ValueObject hierarchy. + // + // FIXME: When we grow m_value, we should represent the added bits as + // undefined somehow instead of as 0's. + if (value_type == Value::eValueTypeHostAddress && + compiler_type.IsValid()) { + if (size_t value_buf_size = m_value.GetBuffer().GetByteSize()) { + size_t value_size = m_value.GetValueByteSize(&m_error, &exe_ctx); + if (m_error.Success() && value_buf_size < value_size) + m_value.ResizeData(value_size); + } + } + Process *process = exe_ctx.GetProcessPtr(); const bool process_is_alive = process && process->IsAlive(); diff --git a/lldb/test/Shell/SymbolFile/DWARF/DW_OP_piece-smaller-than-struct.s b/lldb/test/Shell/SymbolFile/DWARF/DW_OP_piece-smaller-than-struct.s new file mode 100644 index 00000000000..96e4abe110e --- /dev/null +++ b/lldb/test/Shell/SymbolFile/DWARF/DW_OP_piece-smaller-than-struct.s @@ -0,0 +1,110 @@ +# RUN: llvm-mc -filetype=obj -o %t -triple x86_64-pc-linux %s +# RUN: %lldb %t -o "target variable reset" -b | FileCheck %s + +# CHECK: (lldb) target variable reset +# CHECK: (auto_reset) reset = { +# CHECK: ptr = 0xdeadbeefbaadf00d +# Note: We need to find some way to represent "prev" as unknown/undefined. +# CHECK: prev = false +# CHECK: } + + .section .debug_abbrev,"",@progbits + .byte 1 # Abbreviation Code + .byte 17 # DW_TAG_compile_unit + .byte 1 # DW_CHILDREN_yes + .byte 0 # EOM(1) + .byte 0 # EOM(2) + .byte 2 # Abbreviation Code + .byte 52 # DW_TAG_variable + .byte 0 # DW_CHILDREN_no + .byte 3 # DW_AT_name + .byte 8 # DW_FORM_string + .byte 73 # DW_AT_type + .byte 19 # DW_FORM_ref4 + .byte 2 # DW_AT_location + .byte 24 # DW_FORM_exprloc + .byte 0 # EOM(1) + .byte 0 # EOM(2) + .byte 3 # Abbreviation Code + .byte 36 # DW_TAG_base_type + .byte 0 # DW_CHILDREN_no + .byte 3 # DW_AT_name + .byte 8 # DW_FORM_string + .byte 62 # DW_AT_encoding + .byte 11 # DW_FORM_data1 + .byte 11 # DW_AT_byte_size + .byte 11 # DW_FORM_data1 + .byte 0 # EOM(1) + .byte 0 # EOM(2) + .byte 4 # Abbreviation Code + .byte 19 # DW_TAG_structure_type + .byte 1 # DW_CHILDREN_yes + .byte 3 # DW_AT_name + .byte 8 # DW_FORM_string + .byte 11 # DW_AT_byte_size + .byte 11 # DW_FORM_data1 + .byte 0 # EOM(1) + .byte 0 # EOM(2) + .byte 5 # Abbreviation Code + .byte 13 # DW_TAG_member + .byte 0 # DW_CHILDREN_no + .byte 3 # DW_AT_name + .byte 8 # DW_FORM_string + .byte 73 # DW_AT_type + .byte 19 # DW_FORM_ref4 + .byte 56 # DW_AT_data_member_location + .byte 11 # DW_FORM_data1 + .byte 0 # EOM(1) + .byte 0 # EOM(2) + .byte 6 # Abbreviation Code + .byte 15 # DW_TAG_pointer_type + .byte 0 # DW_CHILDREN_no + .byte 73 # DW_AT_type + .byte 19 # DW_FORM_ref4 + .byte 0 # EOM(1) + .byte 0 # EOM(2) + .byte 0 # EOM(3) + + .section .debug_info,"",@progbits +.Lcu_begin0: + .long .Lcu_end-.Lcu_start # Length of Unit +.Lcu_start: + .short 4 # DWARF version number + .long .debug_abbrev # Offset Into Abbrev. Section + .byte 8 # Address Size (in bytes) + .byte 1 # Abbrev [1] 0xb:0x6c DW_TAG_compile_unit +.Lbool: + .byte 3 # Abbrev [3] 0x33:0x7 DW_TAG_base_type + .asciz "bool" # DW_AT_name + .byte 2 # DW_AT_encoding + .byte 1 # DW_AT_byte_size + .byte 2 # Abbrev [2] 0x3a:0x15 DW_TAG_variable + .asciz "reset" # DW_AT_name + .long .Lstruct # DW_AT_type + .byte 2f-1f # DW_AT_location +1: + .byte 0xe # DW_OP_constu + .quad 0xdeadbeefbaadf00d + .byte 0x9f # DW_OP_stack_value + .byte 0x93 # DW_OP_piece + .uleb128 8 + # Note: Only the first 8 bytes of the struct are described. +2: +.Lstruct: + .byte 4 # Abbrev [4] 0x4f:0x22 DW_TAG_structure_type + .asciz "auto_reset" # DW_AT_name + .byte 16 # DW_AT_byte_size + .byte 5 # Abbrev [5] 0x58:0xc DW_TAG_member + .asciz "ptr" # DW_AT_name + .long .Lbool_ptr # DW_AT_type + .byte 0 # DW_AT_data_member_location + .byte 5 # Abbrev [5] 0x64:0xc DW_TAG_member + .asciz "prev" # DW_AT_name + .long .Lbool # DW_AT_type + .byte 8 # DW_AT_data_member_location + .byte 0 # End Of Children Mark +.Lbool_ptr: + .byte 6 # Abbrev [6] 0x71:0x5 DW_TAG_pointer_type + .long .Lbool # DW_AT_type + .byte 0 # End Of Children Mark +.Lcu_end: -- 2.11.0