wesleywiser commented 2 years ago

This is a tracking issue for RFC 3191. The feature gate for the issue is #![feature(debugger_visualizer)].

About tracking issues

Tracking issues are used to record the overall progress of implementation. They are also used as hubs connecting to other relevant issues, e.g., bugs or open design questions. A tracking issue is however not meant for large scale discussion, questions, or bug reports about a feature. Instead, open a dedicated issue for the specific matter and add the relevant feature gate label.

Steps

[x] Implement the RFC
[ ] Adjust documentation (https://github.com/rust-lang/reference/pull/1335)
[ ] Stabilization PR (https://github.com/rust-lang/rust/pull/108668)

Unresolved Questions

No unresolved questions at this time.

Implementation history

[x] Create new attribute #[debugger_viusalizer] and add support for embedding NatVis visualizers through this attribute: https://github.com/rust-lang/rust/pull/91779
[x] Adding support for embedding pretty-printers via new attribute: https://github.com/rust-lang/rust/pull/97028

ridwanabdillahi commented 2 years ago

@rustbot claim

ridwanabdillahi commented 2 years ago

Implementation History

[x] Create new attribute #[debugger_viusalizer] and add support for embedding NatVis visualizers through this attribute: https://github.com/rust-lang/rust/pull/91779
[x] Adding support for embedding pretty-printers via new attribute: https://github.com/rust-lang/rust/pull/97028

madsmtm commented 2 years ago

What's the plan for LLDB? It seems like the design of LLDB is slightly different, they err on the side of "the binary cannot be trusted", so automatically loading these kind of things is not really supported - but maybe it would be possible to provide when using rust-lldb?

pravic commented 1 year ago

Why

#![debugger_visualizer(natvis_file = "../Foo.natvis")]

in Rust code and not

debug_visualizer = "foo.natvis"

in Cargo.toml?

madsmtm commented 1 year ago

Probably because the desire is to allow the attribute directly on types in the future.

gibbyfree commented 1 year ago

@rustbot claim

gibbyfree commented 1 year ago

Request for Stabilization

I'd like to stabilize the debugger_visualizer attribute. A stabilization PR has been submitted as https://github.com/rust-lang/rust/pull/108668.

Summary

The following feature will be stabilized:

The debugger_visualizer attribute:


#![feature(debugger_visualizer)]
#![debugger_visualizer(natvis_file = "foo.natvis")] 
#![debugger_visualizer(gdb_script_file = "foo.py")] 
struct Foo {

}


## Documentation
I've submitted the following PR:
* https://github.com/rust-lang/reference/pull/1335

## Tests
You can find test cases in:
* https://github.com/rust-lang/rust/blob/master/tests/debuginfo/embedded-visualizer.rs - Verifies `debugger_visualizer`'s Natvis output with CDB and pretty print output with GDB
* https://github.com/rust-lang/rust/blob/master/tests/ui/invalid/invalid-debugger-visualizer-option.rs - Invalid file paths in the `debugger_visualizer` attribute result in error
* https://github.com/rust-lang/rust/blob/master/tests/ui/invalid/invalid-debugger-visualizer-target.rs - Applying the `debugger_visualizer` attribute onto a function results in error - the attribute must be applied to a module.

## Real World Usage
NatVis definitions have been added to various popular open-source crates:
* https://github.com/microsoft/windows-rs/pull/2023
* https://github.com/hyperium/http/pull/533
* https://github.com/servo/rust-smallvec/pull/286
* https://github.com/Lokathor/tinyvec/pull/167
* https://github.com/servo/rust-url/pull/783
* https://github.com/starkat99/widestring-rs/pull/29

## Unresolved Issues
No issues are known at this time.

madsmtm commented 1 year ago

I feel like we should figure out the plan for LLDB (or at least rust-lldb) before stabilizing this attribute - it's hard to change after-the-fact if we find that something else is needed to support that.

gibbyfree commented 1 year ago

I feel like we should figure out the plan for LLDB (or at least rust-lldb) before stabilizing this attribute - it's hard to change after-the-fact if we find that something else is needed to support that.

That's a valid point. It might be worth investigating how LLDB handles embedded pretty printers and whether it has support for them. Since the repo already has LLDB debugger tests, we can consider adding tests that target LLDB, try embedding pretty printers and see if they are handled in a similar way as GDB.

That said, I am not sure that this is worth delaying stabilization. At this point, the unstable status of this feature is a barrier to adoption in the Rust community. The out-of-the-box value of this feature relies on crate owners developing their own visualization definitions / pretty printers, and there has been some pushback from crate owners who would rather not support an unstable feature. (See https://github.com/rust-lang/regex/pull/849)

It seems reasonable to me that LLDB support can go through its own stabilization process at a later time. I'm open to further discussion, though.

wesleywiser commented 1 year ago

Thanks for writing up the stabilization report @gibbyfree! Overall this looks good to me. I think we should spend a little time on this point right now though:

It might be worth investigating how LLDB handles embedded pretty printers and whether it has support for them.

If LLDB doesn't support embedded pretty printers, then the overall question is moot and we can go ahead and stabilize the feature for the debuggers which do support this. If LLDB does have support, then I think the information we'll want to know is "how are the pretty printers embedded?" and "how do we enable them automatically?".

Depending on those answers, we can decide if it's important to do that work now (if it is actually possible) or defer until a later time.

gibbyfree commented 1 year ago

Thanks for taking a look @wesleywiser :) I've spent some time in the last week investigating LLDB's pretty printing capabilities.

Usage of debugger_visualizer's natvis_file and gdb_script_file embeds content onto the .pdb/.elf generated by the compiler. This enables crate owners to add debugger visualizations to their crate, which can be utilized by the crate's users without any manual configuration on their end. This scenario is one of debugger_visualizer's primary motivations.

Like GDB, LLDB supports pretty printing of custom types with Python scripts. Unlike GDB, LLDB is incapable of modifying symbol file metadata. This is an important distinction.

We might still be able to improve Rust's LLDB debugging experience. LLDB enables auto-loading of pretty printers by setting an environment variable, so printers can be registered in one's local environment without manual configuration. This is a bit similar to how debugger_visualizer auto-loads printers into GDB. But again, this only affects the local machine and does not embed any data in the debug binary itself.

It may be worth discussing how we choose to add support for more debuggers in the future. Maybe the "embedded" aspect isn't a hard requirement for this feature. Still, I'm inclined to think that that discussion has little bearing on the stabilization of debugger_visualizer's Natvis and GDB support.

This feature has been designed for straight-forward extension, and its current implementation shouldn't block anyone interested in hacking on LLDB support - there were slightly different approaches to supporting Natvis and GDB, and I assume that implementing support for other debuggers will require different approaches too.

That said, I'm not an LLDB expert 😅 I'd love to be educated if I'm mistaken about any of this.

DianaNites commented 1 year ago

Usage of debugger_visualizer's natvis_file and gdb_script_file embeds content onto the .pdb/.elf generated by the compiler. This enables crate owners to add debugger visualizations to their crate, which can be utilized by the crate's users without any manual configuration on their end. This scenario is one of debugger_visualizer's primary motivations.

Is it safe to have untrusted debug visualizers? Can they be malicious?

I did a quick search through this thread and the RFC and its thread for "trust" and "security" and didnt see any discussion of this, but even though Rust already has stuff like proc macros and build.rs, those are only at build time, whereas debug visualizers embedded in a binary could have a greater reach and potential for concern for anyone debugging a Rust binary.

wesleywiser commented 1 year ago

Thanks for investigating @gibbyfree! It sounds like no matter what, we won't be able to provide exactly the same experience in lldb as we do in gdb or WinDbg/VS. We could collect lldb scripts into a well known path relative to the output binary in the target folder and teach rust-lldb to look for this or perhaps a hypothetical cargo debug command could set up lldb with the appropriate environment variables but I don't think any of that work needs to block stabilization of this feature as it currently exists as none of that would imply changes to gdb pretty printers or the Natvis support.

@DianaNites good question! There are two different kinds of debugger visualizers supported currently: Natvis and gdb pretty printers.

Natvis is a declarative language that only allows you to provide an alternative description of a type. The number of supported operations is very small and I think the most an adversarial visualizer could do would be to attempt to obscure or mislead the debugger user regarding some value in the debugger. Users are able to turn off the visualization either global or per value in supported debuggers so I don't believe there's a realistic path of attack via this avenue.

Gdb pretty printers are Python scripts that run inside the embedded Python interpreter in gdb. This is an optional feature of gdb that most large distros seem to enable. I believe (but have not tested) that scripts are not sandboxed in any sort of way. As such, they could access resources on the machine hosting the debugger. gdb itself provides mechanisms to enable or disable autoloading of embedded debugger scripts. I think for most users, this is likely not an additional path of attack as they have already compiled attacker-controlled code (which could execute malicious code on their machine via proc macros or build.rs as you pointed out) and they have also run the binary which contains attacker-controlled code as well. For users with additional security needs, gdb should be configured to not autoload scripts but this is not a Rust specific concern.

Does that make sense?

wesleywiser commented 1 year ago

I think this is ready for stabilization.

@gibbyfree wrote a stabilization report and the associated PR is #108668.

@rfcbot fcp merge

rfcbot commented 1 year ago

Team member @wesleywiser has proposed to merge this. The next step is review by the rest of the tagged team members:

[ ] @Aaron1011
[x] @cjgillot
[x] @davidtwco
[x] @eddyb
[x] @estebank
[x] @joshtriplett
[x] @lcnr
[x] @matthewjasper
[x] @michaelwoerister
[x] @nagisa
[x] @nikomatsakis
[x] @oli-obk
[x] @petrochenkov
[x] @pnkfelix
[x] @scottmcm
[ ] @tmandry
[x] @wesleywiser

No concerns currently listed.

Once a majority of reviewers approve (and at most 2 approvals are outstanding), this will enter its final comment period. If you spot a major issue that hasn't been raised at any point in this process, please speak up!

cc @rust-lang/lang-advisors: FCP proposed for lang, please feel free to register concerns. See this document for info about what commands tagged team members can give me.

nikomatsakis commented 1 year ago

@rfcbot fcp reviewed -- better debugger experience ftw!

petrochenkov commented 1 year ago

I still don't like how the visualizer file is looked up (based on attr.span, https://github.com/rust-lang/rfcs/pull/3191#issuecomment-1067206269), but the same problem is shared by macros like include_str and I don't expect #[debugger_visualizer] to be used "creatively" enough to prevent a fix in the future.

rfcbot commented 1 year ago

:bell: This is now entering its final comment period, as per the review above. :bell:

rfcbot commented 1 year ago

The final comment period, with a disposition to merge, as per the review above, is now complete.

As the automated representative of the governance process, I would like to thank the author for their work and everyone else who contributed.

This will be merged soon.

ehuss commented 1 year ago

I'm concerned about the stabilization of this feature in its current state. I filed a few issues (see https://github.com/rust-lang/rust/labels/F-debugger_visualizer). It is almost unusable for the purpose of authoring visualizers since you can't modify a file without needing to delete your build directory and starting over. I would recommend looking at either fixing those issues or considering to revert the stabilization, since otherwise I think it would provide a poor first impression.

lcnr commented 1 year ago

nominating for t-compiler meeting due to the above concern. if we're able to remove this asynchronously before then we can always remove the nomination.

michaelwoerister commented 1 year ago

I agree that these issue are a blocking concern for the feature. If I'm reading the schedule right, we have until May 26 to either fix the issue or revert stabilization.

michaelwoerister commented 1 year ago

Thanks for opening the issues, @ehuss! I've assigned myself to do an initial investigation.