This is an archive of the discontinued LLVM Phabricator instance.

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

Authored by vsk on Jan 21 2020, 4:19 PM.

Details

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

Diff Detail

Event Timeline

vsk created this revision.Jan 21 2020, 4:19 PM

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 added a comment.Jan 21 2020, 5:29 PM

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 accepted this revision.Jan 21 2020, 7:18 PM
In D73148#1832762, @vsk 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.

This revision is now accepted and ready to land.Jan 21 2020, 7:18 PM

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

vsk planned changes to this revision.Jan 22 2020, 3:03 PM

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

vsk updated this revision to Diff 239753.Jan 22 2020, 5:54 PM
vsk edited the summary of this revision. (Show Details)
This revision is now accepted and ready to land.Jan 22 2020, 5:54 PM
vsk requested review of this revision.Jan 22 2020, 5:55 PM
vsk added a reviewer: jasonmolenda.
labath added a subscriber: labath.Jan 22 2020, 11:32 PM
In D73148#1834955, @vsk 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?

In D73148#1834955, @vsk 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 ?

Yes you have. Sorry for the noise.

aprantl added inline comments.Jan 24 2020, 2:31 PM
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 updated this revision to Diff 241602.Jan 30 2020, 3:16 PM
vsk retitled this revision from [lldb/Value] Report size of Value as size of underlying data buffer to [lldb/Value] Avoid reading more data than the host has available.
vsk edited the summary of this revision. (Show Details)

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 marked an inline comment as done.Jan 30 2020, 3:18 PM
vsk added inline comments.
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 planned changes to this revision.Jan 30 2020, 6:05 PM

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 added a subscriber: shafik.Jan 31 2020, 8:52 AM
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 updated this revision to Diff 241814.Jan 31 2020, 2:29 PM
vsk edited the summary of this revision. (Show Details)
  • 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.
clayborg accepted this revision.Jan 31 2020, 3:52 PM
This revision is now accepted and ready to land.Jan 31 2020, 3:52 PM
This revision was automatically updated to reflect the committed changes.
Herald added a project: Restricted Project. · View Herald TranscriptJan 31 2020, 4:35 PM