[lldb/Value] Avoid reading more data than the host has available

[vsk]

Summary

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

[clayborg]

Would it be better to just ensure that the buffer for DW_OP_piece is at least the size of the type instead? To do this right we would really need to track which bytes in the buffer are actually valid. This patch assumes we will always get the first N bytes of a value. Is it possible to describe bytes 4:7 of a 16 bit value where bits 0:3 and 8:15 are not valid? In this case, with this patch, we would think that bytes 0:3 are valid when they are just not specified, 4:7 would be valid, and 8:15 would not display because our value doesn't contain the value.

[vsk]

In D73148#1832704, @clayborg wrote:

Would it be better to just ensure that the buffer for DW_OP_piece is at least the size of the type instead? To do this right we would really need to track which bytes in the buffer are actually valid. This patch assumes we will always get the first N bytes of a value. Is it possible to describe bytes 4:7 of a 16 bit value where bits 0:3 and 8:15 are not valid? In this case, with this patch, we would think that bytes 0:3 are valid when they are just not specified, 4:7 would be valid, and 8:15 would not display because our value doesn't contain the value.

It's not necessary for DW_OP_piece to produce a type-sized result buffer, as the description of an object must start at byte offset 0, and we represent the undefined bits within the Value either with 0 or by leaving them out. E.g. Evaluate({DW_OP_piece, 1, DW_OP_const1u, 0xff, DW_OP_piece, 1, DW_OP_piece, 1}) produces the 24-bit scalar 0x00ff00, where the 0's correspond to the undefined low bits and the undefined high bits are left out. It /is/ a bug that we use 0 for undefined bits instead of u (or something), but we don't need a type-sized buffer to fix that (a bitset would suffice). Finding a way to improve the representation of undefined bits is something I'm interested in, but it seems like a separate problem, and it's more urgent for me to fix the memory smasher.

I'm also not sure that making DW_OP_piece produce a type-sized result would be a sufficient fix, as there (theoretically) may be clients of Value::ResizeData() other than the DW_OP_piece logic. I just count one, though (ExpressionVariable::GetValueBytes()), and I'm not sure whether it can resize the buffer to something other than the underlying type size.

[clayborg]

In D73148#1832762, @vsk wrote:

In D73148#1832704, @clayborg wrote:

Would it be better to just ensure that the buffer for DW_OP_piece is at least the size of the type instead? To do this right we would really need to track which bytes in the buffer are actually valid. This patch assumes we will always get the first N bytes of a value. Is it possible to describe bytes 4:7 of a 16 bit value where bits 0:3 and 8:15 are not valid? In this case, with this patch, we would think that bytes 0:3 are valid when they are just not specified, 4:7 would be valid, and 8:15 would not display because our value doesn't contain the value.

It's not necessary for DW_OP_piece to produce a type-sized result buffer, as the description of an object must start at byte offset 0, and we represent the undefined bits within the Value either with 0 or by leaving them out. E.g. Evaluate({DW_OP_piece, 1, DW_OP_const1u, 0xff, DW_OP_piece, 1, DW_OP_piece, 1}) produces the 24-bit scalar 0x00ff00, where the 0's correspond to the undefined low bits and the undefined high bits are left out. It /is/ a bug that we use 0 for undefined bits instead of u (or something), but we don't need a type-sized buffer to fix that (a bitset would suffice). Finding a way to improve the representation of undefined bits is something I'm interested in, but it seems like a separate problem, and it's more urgent for me to fix the memory smasher.

I'm also not sure that making DW_OP_piece produce a type-sized result would be a sufficient fix, as there (theoretically) may be clients of Value::ResizeData() other than the DW_OP_piece logic. I just count one, though (ExpressionVariable::GetValueBytes()), and I'm not sure whether it can resize the buffer to something other than the underlying type size.

Thanks for the clarification. This will work for now.

[clayborg]

Actually it would be nice to have a test that will trigger on at least one build bot that runs ASAN?

[vsk]

In D73148#1832897, @clayborg wrote:

Actually it would be nice to have a test that will trigger on at least one build bot that runs ASAN?

I'll add an end-to-end test for DW_OP_piece, though I worry it might be brittle.

As I was testing this more, I realized that CommandObjectFrameVariable may construct an empty Value and call GetValueByteSize on it. This patch breaks that code path because it makes the reported size 0 (instead of whatever GetCompilerType().GetByteSize() would be). I'm not sure what the best fix is. Some options:

1. Special-case 0-sized eValueTypeHostAddress Values, and fall back to the compiler type in this case.
2. Just make DW_OP_piece produce a type-sized buffer (by padding with zeroes when needed), as Greg suggested earlier.

Any advice appreciated. I'm leaning towards (1) as it's a simpler fix (and I've tested it out on an end-to-end example).

[labath]

In D73148#1834955, @vsk wrote:

In D73148#1832897, @clayborg wrote:

Actually it would be nice to have a test that will trigger on at least one build bot that runs ASAN?

I'll add an end-to-end test for DW_OP_piece, though I worry it might be brittle.

I don't think that's a good way to write a test like this. I suggest checking out test/Shell/SymbolFile/DWARF/DW_OP_piece-struct.s for an dwarf expression test that does not rely on guessing the DW_OP opcodes used by the compiler. Maybe you can just take that test and tweak it to do what you need?

[aprantl]

In D73148#1835488, @labath wrote:

In D73148#1834955, @vsk wrote:

In D73148#1832897, @clayborg wrote:

Actually it would be nice to have a test that will trigger on at least one build bot that runs ASAN?

I'll add an end-to-end test for DW_OP_piece, though I worry it might be brittle.

I don't think that's a good way to write a test like this. I suggest checking out test/Shell/SymbolFile/DWARF/DW_OP_piece-struct.s for an dwarf expression test that does not rely on guessing the DW_OP opcodes used by the compiler. Maybe you can just take that test and tweak it to do what you need?

Sorry, I haven't read the rest yet, but for testing: Have you seen lldb/unittests/Expression/DWARFExpressionTest.cpp ?

[aprantl]

Yes you have. Sorry for the noise.

[aprantl]

lldb/packages/Python/lldbsuite/test/functionalities/dw_op_piece/main.cpp

34 ↗

(On Diff #239753)

It seems unlikely that this lowering survives very long. Could you move this to unit tests instead?

[vsk]

Address review feedback:

• Add a test that doesn't depend on the compiler.
• Convert the old test into a simpler regression test.
• Based on feedback from Jim, don't try to change the definition of a Value's size, as the expectation that the type is correct is baked in deeply.

[vsk]

lldb/packages/Python/lldbsuite/test/functionalities/dw_op_piece/main.cpp

34 ↗

(On Diff #239753)

I'm not sure how to write this as a unit test. I expect that the added regression test + assembly test covers what we want to cover, though.

[vsk]

Yikes, this version seems to break TestClassTemplateParameterPack.py (and probably other tests as well). Maybe we expect to be able to read past the end of the buffer inside of a ValueObjectChild, because the buffer for the next ValueObjectChild is supposed to be there?

[shafik]

In D73148#1850978, @vsk wrote:

Yikes, this version seems to break TestClassTemplateParameterPack.py (and probably other tests as well). Maybe we expect to be able to read past the end of the buffer inside of a ValueObjectChild, because the buffer for the next ValueObjectChild is supposed to be there?

Or they could be real bugs?

[vsk]

• Go with @gclayton's original suggestion, as it's proven to be impractical to alter the notion of a Value's size in the Value/ValueObject logic.
• I've tested this and gotten a clean check-lldb run. The added tests now pass ASan-clean.

lldb/packages/Python/lldbsuite/test/functionalities/optimized_code/Makefile

+CXX_SOURCES := main.cpp
+CXXFLAGS_EXTRAS := -O2
+include Makefile.rules

lldb/packages/Python/lldbsuite/test/functionalities/optimized_code/TestNoASanExceptionAfterEvalOP_piece.py

+from lldbsuite.test import lldbinline
+from lldbsuite.test import decorators
+
+lldbinline.MakeInlineTest(__file__, globals())

lldb/packages/Python/lldbsuite/test/functionalities/optimized_code/main.cpp

+// 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;
+}

lldb/source/Core/ValueObjectVariable.cpp

if (expr.Evaluate(&exe_ctx, nullptr, loclist_base_load_addr, nullptr,
       m_value.SetContext(Value::eContextTypeVariable, variable);
 
       CompilerType compiler_type = GetCompilerType();
       if (compiler_type.IsValid())
         m_value.SetCompilerType(compiler_type);
 
       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();
 
       switch (value_type) {
       case Value::eValueTypeVector:
       // fall through
       case Value::eValueTypeScalar:
         // The variable value is in the Scalar value inside the m_value. We can

lldb/test/Shell/SymbolFile/DWARF/DW_OP_piece-smaller-than-struct.s

+# 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: