Page MenuHomePhabricator

[lldb/DWARF] Fix evaluator crash when accessing empty stack
ClosedPublic

Authored by mib on Mar 30 2020, 5:37 PM.

Details

Summary

This patch fixes a crash that happens on the DWARF expression evaluator
when trying to access the top of the stack while it's empty.

Signed-off-by: Med Ismail Bennani <medismail.bennani@gmail.com>

Diff Detail

Event Timeline

mib created this revision.Mar 30 2020, 5:37 PM
aprantl accepted this revision.Mar 30 2020, 8:28 PM

This is obviously good! Do you think that a similar error handling bug might exist in other cases that depend top-of-stack?

This revision is now accepted and ready to land.Mar 30 2020, 8:28 PM
labath added a subscriber: labath.Mar 31 2020, 1:02 AM

This is obviously good! Do you think that a similar error handling bug might exist in other cases that depend top-of-stack?

Most DW_OP cases check their stack, but it's quite possible that others were missed too. It might be a nice cleanup to make a function like (getMinimalStackSize(op)) and move this check up in front of the big switch. That could not handle all operators, as for some of them, the value is not statically known, but it would handle the vast majority of them.

lldb/unittests/Expression/DWARFExpressionTest.cpp
238

EXPECT_THAT_EXPECTED(Evaluate(...), Failed()) is better, as it will produce an error (instead of a crash) in case the evaluation does succeed for any reason.

kwk added a subscriber: kwk.Mar 31 2020, 2:09 AM

This is obviously good! Do you think that a similar error handling bug might exist in other cases that depend top-of-stack?

Most DW_OP cases check their stack, but it's quite possible that others were missed too. It might be a nice cleanup to make a function like (getMinimalStackSize(op)) and move this check up in front of the big switch. That could not handle all operators, as for some of them, the value is not statically known, but it would handle the vast majority of them.

@labath I somewhat like that the logic for each op is close to the case statement. Creating the function that you suggested would separate this check out somewhere else and you would have to look at two places. At least for code review, the current solution is much clearer to me.

In D77108#1951997, @kwk wrote:

Most DW_OP cases check their stack, but it's quite possible that others were missed too. It might be a nice cleanup to make a function like (getMinimalStackSize(op)) and move this check up in front of the big switch. That could not handle all operators, as for some of them, the value is not statically known, but it would handle the vast majority of them.

@labath I somewhat like that the logic for each op is close to the case statement. Creating the function that you suggested would separate this check out somewhere else and you would have to look at two places. At least for code review, the current solution is much clearer to me.

I don't have a problem with the current patch (modulo the test tweak) -- it fixes a real problem and it follows the style of the surrounding code. However, DWARFExpression::Evaluate is gigantic (2600LOC), and nearly half of that is error handling. Tastes may vary, but I think that's a bigger readability problem than having to look at two places to understand an opcode.

mib updated this revision to Diff 253895.Mar 31 2020, 8:25 AM
mib marked an inline comment as done.

Addressed Pavel's request.

mib added a comment.EditedMar 31 2020, 8:27 AM

This is obviously good! Do you think that a similar error handling bug might exist in other cases that depend top-of-stack?

I quickly ran through the function, and it seems this was the only location where we were missing the check.

This revision was automatically updated to reflect the committed changes.

For the future, a clean solution would be extending the macros in Dwarf.def to list the stack effects in the definitions of the DW_OP_*, for example

// opcode, name, version, vendor, in, out
HANDLE_DW_OP(0x12, dup, 2, DWARF, 1, 2)

and then we could write a static verifier that ensures that the stack effects of an entire expression is sound. (And we could check this in LLVM, already, too).

Is there any DWARF operator where the stack effect (number of consumes & produced stack elements) isn't statically known?

aprantl added subscribers: probinson, dblaikie, djtodoro and 3 others.

CC'ing the DWARF cabal.

For the future, a clean solution would be extending the macros in Dwarf.def to list the stack effects in the definitions of the DW_OP_*, for example

// opcode, name, version, vendor, in, out
HANDLE_DW_OP(0x12, dup, 2, DWARF, 1, 2)

That sounds very interesting. However,

Is there any DWARF operator where the stack effect (number of consumes & produced stack elements) isn't statically known?

DW_OP_pick comes to mind. It always produces one item, but it can reach arbitrarily deep into the dwarf stack (without consuming that item). Glancing through the dwarf spec, it looks like this is the only such operator, so special-casing that wouldn't be too bad...

For the future, a clean solution would be extending the macros in Dwarf.def to list the stack effects in the definitions of the DW_OP_*, for example

// opcode, name, version, vendor, in, out
HANDLE_DW_OP(0x12, dup, 2, DWARF, 1, 2)

and then we could write a static verifier that ensures that the stack effects of an entire expression is sound. (And we could check this in LLVM, already, too).

DW_OP_entry_value may be requiring a special handling as well.

kwk added a comment.Apr 2 2020, 3:43 AM
In D77108#1951997, @kwk wrote:

Most DW_OP cases check their stack, but it's quite possible that others were missed too. It might be a nice cleanup to make a function like (getMinimalStackSize(op)) and move this check up in front of the big switch. That could not handle all operators, as for some of them, the value is not statically known, but it would handle the vast majority of them.

@labath I somewhat like that the logic for each op is close to the case statement. Creating the function that you suggested would separate this check out somewhere else and you would have to look at two places. At least for code review, the current solution is much clearer to me.

I don't have a problem with the current patch (modulo the test tweak) -- it fixes a real problem and it follows the style of the surrounding code. However, DWARFExpression::Evaluate is gigantic (2600LOC), and nearly half of that is error handling. Tastes may vary, but I think that's a bigger readability problem than having to look at two places to understand an opcode.

@labath okay, that makes sense. Sorry.