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
_______________________________________________
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
