[Lldb-commits] [PATCH] D153840: [LLDB] Fix buffer overflow problem in DWARFExpression::Evaluate.

[David Blaikie via Phabricator via lldb-commits]

dblaikie added a comment.

In D153840#4474213 <https://reviews.llvm.org/D153840#4474213>, @cmtice wrote:

> Hi Jason,
>
> I had been talking more with David, and yes, I had come to the conclusion that you are both right and that this was not the right fix.  I am planning on reverting this, but I am trying to figure out the right fix to replace it with.  I can't share the source that was causing the bug to manifest, because it's in proprietary code, but David is looking at it and I believe he has come to the conclusion that there is a bug in the DWARF code generation -- we were getting a size of 16, which is absolutely not right.  The question is, in the case of bad DWARF being generated, what (if anything) should the LLDB code here be doing? Should we check the size as soon as we read it in, and assert that  it must be <= 8?  Or something else?  Or just leave the LLDB code entirely alone?
>
> What do you (and other reviewers) think is the right thing to do here?

While it's likely generally under-tested/under-covered, debuggers shouldn't crash on invalid/problematic DWARF. So this code should probably abort parsing an invalid expression like this - there's probably some codepaths through here that do some kind of error handling like the "Failed to dereference pointer from" a few lines later. I'd expect a similar error handling should be introduced if `size` is invalid (not sure if "invalid" should be `> sizeof(lldb::addr_t)` or maybe something more specific (like it should check the address size in the DWARF, perhaps - I don't know/recall the /exact/ DWARF spec wording about the size limitations)).



================
Comment at: lldb/source/Expression/DWARFExpression.cpp:1088
         intptr_t ptr;
         ::memcpy(&ptr, src, sizeof(void *));
         // I can't decide whether the size operand should apply to the bytes in
----------------
FWIW, I think this code is differently invalid, but figured I'd mention it while I'm here - this is probably illegal load widening. If we're processing `DW_OP_deref_size(1)` maybe that 1 byte is at the end of a page - reading `sizeof(void*)` would read more data than would be valid, and be a problem?


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D153840/new/

https://reviews.llvm.org/D153840