[Debug Info] Gracefully handle invalid String/Vec#155509
[Debug Info] Gracefully handle invalid String/Vec#155509Walnut356 wants to merge 2 commits intorust-lang:mainfrom
String/Vec#155509Conversation
rustbot
added
the
S-waiting-on-review
|
rustbot has assigned @Mark-Simulacrum. Use Why was this reviewer chosen?The reviewer was selected based on:
|
|
|
||
| if length <= 0: | ||
| return '""' | ||
|
|
||
| no_hi_bit_max: int = 1 << ((pointer.GetByteSize() * 8) - 1) | ||
| # technically length isn't a NoHighBit<usize>, but if length should always be <= capacity |
There was a problem hiding this comment.
| # technically length isn't a NoHighBit<usize>, but if length should always be <= capacity | |
| # technically length isn't a NoHighBit<usize>, but length should always be <= capacity |
Not sure what if was meaning to refer to here?
| else: | ||
| raise Exception("ReadMemory error: %s", error.GetCString()) | ||
| try: | ||
| data = process.ReadMemory(pointer.GetValueAsUnsigned(), length, error) |
There was a problem hiding this comment.
Should we aim for all process.ReadMemory calls to be wrapped like this? Maybe we should define our own helper to push for that? E.g., StdPathSummaryProvider probably needs the same treatment?
There was a problem hiding this comment.
Ah, yeah that's a good question. It's probably best to, since swig FFI exceptions cause such catastrophic issues.
StdPathSummaryProvider probably needs the same treatment
Atm Path and OsStr providers defer directly to the vec synthetic's get_num_children and get_child_at_index. Since the vec synthetic can now detect invalid state, it'll just report an empty string.
It wouldn't be a bad idea to switch to using process.ReadMemory eventually since it's significantly more performant, but it's not too urgent since paths are usually quite short.
There was a problem hiding this comment.
rust/src/etc/lldb_providers.py
Line 368 in 2f43fe4
There was a problem hiding this comment.
🤦♂️ i might be blind lol
rustbot
removed
the
S-waiting-on-review
|
Reminder, once the PR becomes ready for a review, use |
rustbot
added
the
S-waiting-on-author
|
This PR was rebased onto a different main commit. Here's a range-diff highlighting what actually changed. Rebasing is a normal part of keeping PRs up to date, so no action is needed—this note is just to help reviewers. |
|
@rustbot ready |
rustbot
added
S-waiting-on-review
3 participants
Somewhat related to #150392.
Currently the handling can throw an exception, which we should absolutely not do. It causes issues with debugger adapters (e.g. CodeLLDB will hang forever. Trying to stop the debugger via vscode's interface causes a CodeLLDB to leak memory constantly until RAM is depleted and the OS starts killing processes). The exception has been replaced with a printed error message and a placeholder value.
Additionally, if a String/Vec is in an "invalid" state due to niche optimization (
capacity >= (1 << 63), common withOption<String>/Option<Vec<T>>), the pointer and length values will be meaningless, but are not guaranteed to be 0'd. The debugger will happily proceed as if they are useful values, and often do things like <try to read multiple GB of data from the debugee>.I added simple checks to ensure that the capacity and length are within bounds, and that the pointer is non-null. If any check fails, the string/vec just acts as if it's empty.
Eventually this problem will be solved on LLDB's end via llvm/llvm-project#188487 or similar, but preventing issues on our end in the short term will help a lot.