[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

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

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/dw_op_piece/Makefile

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

lldb/packages/Python/lldbsuite/test/functionalities/dw_op_piece/TestDW_OP_piece.py

+from lldbsuite.test import lldbinline
+from lldbsuite.test import decorators
+
+lldbinline.MakeInlineTest(__file__, globals(),
+        [decorators.skipUnlessArch('x86_64')])

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

+// The point of this test is to exercise lldb's DW_OP_piece evaluation logic.
+// The CHECK lines in the test depend on aspects of the optimizer's behavior
+// that aren't guaranteed. If the test fails after a compiler change, run it in
+// trace mode (./bin/lldb-dotest -t ...): this should print out the new location
+// expressions.
+
+__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;
+
+  // 1) Check that "v1" is described by 3 pieces (one each for "f1" and "f2"
+  // (in registers), and a third for padding (a literal)).
+  //
+  //% self.filecheck("image lookup -va $pc", "main.cpp", "-check-prefix=INFO-V1")
+  // INFO-V1: name = "v1", type = "S1", location = DW_OP_reg0 RAX, DW_OP_piece 0x4, DW_OP_piece 0x4, DW_OP_lit0, DW_OP_stack_value, DW_OP_piece 0x8, decl
+  sink++;
+
+  // 2) Check that lldb displays "v1" correctly.
+  //
+  //% self.filecheck("frame variable v1", "main.cpp", "-check-prefix=SHOW-V1")
+  // SHOW-V1:      (S1) v1 = {
+  // SHOW-V1-NEXT:   f1 = 0
+  // SHOW-V1-NEXT:   f2 = 0x{{0+$}}
+  // SHOW-V1-NEXT: }
+
+  S2 v2(v1.f1);
+
+  // 3) Check that "v2" is described by 2 pieces (one each for "a" and "b").
+  // Note that a piece for "pad" is left out: this is crucial, as it means that
+  // DWARFExpression::Evaluate() will produce a 2-byte Value. We want to test
+  // that lldb can handle the Value without smashing memory (this is a
+  // regression test for a bug found by ASan).
+  sink += use(v2.a);
+  //% self.filecheck("image lookup -va $pc", "main.cpp", "-check-prefix=INFO-V2")
+  // INFO-V2: name = "v2", type = "S2", location = DW_OP_piece 0x1, DW_OP_lit0, DW_OP_stack_value, DW_OP_piece 0x1, decl
+
+  // 4) Check that lldb displays "v2" correctly.
+  sink += use(v2.pad);
+  //% self.filecheck("frame variable v2", "main.cpp", "-check-prefix=SHOW-V2")
+  // SHOW-V2: (S2) v2 = (a = '\0', b = '\0', pad = {{.*}})
+
+  return 0;
+}

lldb/source/Core/Value.cpp

if (GetRegisterInfo()) {
       return GetRegisterInfo()->byte_size;
     }
     break;
 
   case eContextTypeInvalid:
   case eContextTypeLLDBType: // Type *
   case eContextTypeVariable: // Variable *
   {
+    // The size of this Value may be less than the size of the type of its
+    // source variable due to truncating operations such as DW_OP_piece.
+    if (m_value_type == eValueTypeHostAddress)
+      if (uint64_t buffer_size = GetBuffer().GetByteSize())
+        return buffer_size;
+
     auto *scope = exe_ctx ? exe_ctx->GetBestExecutionContextScope() : nullptr;
     if (llvm::Optional<uint64_t> size = GetCompilerType().GetByteSize(scope)) {
       if (error_ptr)
         error_ptr->Clear();
       return *size;
     }
     break;
   }

lldb/unittests/Expression/DWARFExpressionTest.cpp

TEST(DWARFExpression, DW_OP_piece) {
   EXPECT_THAT_EXPECTED(Evaluate({DW_OP_const2u, 0x11, 0x22, DW_OP_piece, 2,
                                  DW_OP_const2u, 0x33, 0x44, DW_OP_piece, 2}),
                        llvm::HasValue(GetScalar(32, 0x44332211, true)));
   EXPECT_THAT_EXPECTED(
       Evaluate({DW_OP_piece, 1, DW_OP_const1u, 0xff, DW_OP_piece, 1}),
       // Note that the "00" should really be "undef", but we can't
       // represent that yet.
       llvm::HasValue(GetScalar(16, 0xff00, true)));
+
+  for (unsigned char ByteSize = 1; ByteSize <= 8; ++ByteSize) {
+    llvm::Expected<Scalar> empty = Evaluate({DW_OP_piece, ByteSize});
+    // Note that the "0" should really be "undef", but we can't
+    // represent that yet.
+    EXPECT_THAT_EXPECTED(empty,
+                         llvm::HasValue(GetScalar(ByteSize * 8, 0, true)));
+
+    Value pieces;
+    ASSERT_EQ(pieces.AppendDataToHostBuffer(empty.get()), ByteSize);
+    ASSERT_EQ(pieces.GetValueByteSize(nullptr, nullptr), ByteSize);
+  }
+
+  // When CommandObjectFrameVariable constructs an empty Value, check that
+  // we do not successfully report its size as 0. A CompilerType is needed to
+  // get the size in this special case.
+  {
+    Value empty;
+    empty.ResizeData(0);
+    Status error;
+    ASSERT_EQ(empty.GetValueByteSize(&error, nullptr), 0);
+    ASSERT_TRUE(error.Fail());
+  }
 }