feat: improve user-mode and kernel-mode driver panic handlers#642
feat: improve user-mode and kernel-mode driver panic handlers#642Alan632 wants to merge 18 commits into
Conversation
…rnel panic handler and clean up documentation
There was a problem hiding this comment.
Pull request overview
Adds improved panic handling for Windows Drivers Rust (WDR) targets: kernel-mode panics now trigger a bugcheck with source-location metadata, and UMDF drivers can install a std panic hook to emit panic info to the debugger.
Changes:
- Implement WDM/KMDF panic handler that calls
KeBugCheckExwith panic location parameters. - Add UMDF-only
install_panic_hook()to print panic info (and break into debugger in debug builds). - Add
wdk-panicbuild script + dependencies and update UMDF template to install the hook.
Reviewed changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| crates/wdk-panic/src/lib.rs | Adds kernel bugcheck panic handler + UMDF panic hook installer and updates crate docs/cfgs. |
| crates/wdk-panic/build.rs | Adds build script to emit WDK cfg settings for conditional compilation. |
| crates/wdk-panic/Cargo.toml | Adds wdk dependency and wdk-build build-dependency. |
| crates/cargo-wdk/templates/umdf/lib.rs.tmp | Calls wdk_panic::install_panic_hook() in UMDF DriverEntry. |
| crates/cargo-wdk/templates/umdf/Cargo.toml.tmp | Adds wdk-panic dependency to UMDF template. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| [dependencies] | ||
| wdk.workspace = true | ||
|
|
||
| [build-dependencies] | ||
| wdk-build.workspace = true |
There was a problem hiding this comment.
wdk-panic now adds a new public API (install_panic_hook) and changes panic behavior for WDM/KMDF (bugcheck instead of an infinite loop), but the crate version remains 0.4.1. Please bump the crate version (and update the changelog in the release process) so downstream consumers/templates can depend on a version that actually contains this API/behavior.
There was a problem hiding this comment.
version bumps happen in release prs
|
|
||
| [dependencies] | ||
| wdk = "0.4.1" | ||
| wdk-panic = "0.4.1" |
There was a problem hiding this comment.
The UMDF template pins wdk-panic = "0.4.1", but this PR introduces the install_panic_hook API. If a user generates a project and pulls from crates.io, it will fail to compile unless the template is updated to the new wdk-panic version (or the template version is substituted during release).
| wdk-panic = "0.4.1" | |
| wdk-panic = "0.5.1" |
There was a problem hiding this comment.
Will update after release.
There was a problem hiding this comment.
Can this not be added now? i think we had infra to not run certain tests in release prs
There was a problem hiding this comment.
That logic works only in release-plz PRs so we can't switch to 0.5.1 in this PR. We have to wait until the next release.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 5 out of 6 changed files in this pull request and generated 2 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| pub unsafe extern "system" fn driver_entry( | ||
| _driver: PDRIVER_OBJECT, | ||
| _registry_path: PCUNICODE_STRING, | ||
| ) -> NTSTATUS { |
There was a problem hiding this comment.
wdk_panic::install_panic_hook() is only defined under #[cfg(all(not(test), driver_model__driver_type = "UMDF"))]. Calling it unconditionally from driver_entry will fail to compile when building with cfg(test) (e.g., cargo test). Gate the call with #[cfg(not(test))] (or relax the cfg on install_panic_hook) so test builds don’t break.
| ) -> NTSTATUS { | |
| ) -> NTSTATUS { | |
| #[cfg(not(test))] |
…e_case Windows API
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 5 out of 6 changed files in this pull request and generated 4 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com> Signed-off-by: Alan632 <aln.noda7@gmail.com>
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 3 out of 4 changed files in this pull request and generated 1 comment.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 4 out of 5 changed files in this pull request and generated 1 comment.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #642 +/- ##
==========================================
+ Coverage 78.44% 79.45% +1.00%
==========================================
Files 25 26 +1
Lines 5201 5500 +299
Branches 5201 5500 +299
==========================================
+ Hits 4080 4370 +290
Misses 1001 1001
- Partials 120 129 +9 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 4 out of 5 changed files in this pull request and generated 1 comment.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| /// Registers a panic hook for UMDF drivers that prints panic information via | ||
| /// [`wdk::println!`] and then aborts the host process. | ||
| /// | ||
| /// Output is routed through [`OutputDebugStringA`], which is received by any |
There was a problem hiding this comment.
Rustdoc link [OutputDebugStringA] is unqualified and likely resolves to no item in this crate, which will trigger broken_intra_doc_links (docs CI uses RUSTDOCFLAGS=-D warnings). Prefer either fully qualifying it (e.g., wdk_sys::windows::OutputDebugStringA with an appropriate direct dependency) or formatting it as code (backticks) instead of an intra-doc link.
| /// Output is routed through [`OutputDebugStringA`], which is received by any | |
| /// Output is routed through `OutputDebugStringA`, which is received by any |
| #[cfg(all(not(test), driver_model__driver_type = "UMDF"))] | ||
| pub fn install_panic_hook() { | ||
| std::panic::set_hook(Box::new(|info| { | ||
| wdk::println!("[PANIC] {info}"); |
There was a problem hiding this comment.
do you have a sample output of how this looks like when you have a debugger attached to the umdf driver?
|
|
||
| // High, uncommon bugcheck code to avoid collision with OS-defined identifiers. | ||
| // Lower 24 bits spell `PNC` (0x50 'P', 0x4E 'N', 0x43 'C') for recognizability. | ||
| const BUGCHECK_RUST_CODE: u32 = 0x8050_4E43; |
There was a problem hiding this comment.
I think this reads better if we derive it from the ASCII tag instead of spelling the hex literal directly. Something like const RUST_PANIC_BUGCHECK_CODE: u32 = u32::from_be_bytes(*b"RPNC"); keeps the same value but makes the intent much clearer. Also i think starting with 0x8 is not provably necessary.
gurry
left a comment
There was a problem hiding this comment.
LGTM. Just one minor comment on a safety comment.
| // SAFETY: `KeBugCheckEx` is a Windows kernel API exported by `ntoskrnl.exe` | ||
| // that is callable at any IRQL and never returns (it halts the system with a | ||
| // bugcheck). The parameters are scalar `usize` values recorded in the crash | ||
| // dump; `KeBugCheckEx` does not dereference `panic_filename_ptr` — it is stored | ||
| // as an opaque value for post-mortem analysis. The FFI signature matches the | ||
| // WDK declaration in `wdm.h`. This call is sound because: | ||
| // 1. The function is always available in kernel mode (linked via ntoskrnl.lib). | ||
| // 2. The calling convention is correct (`extern "system"` maps to the | ||
| // appropriate Windows calling convention for the target architecture). | ||
| // 3. The `-> !` return type is upheld — `KeBugCheckEx` never returns. |
There was a problem hiding this comment.
A safety comment only needs to explain that you are calling the function correctly and upholding its pre-conditions, e.g. arguments being passed are valid, lifetimes are not violoated etc. It need not explain that the function is declared correctly which this comment seems to do.
(Assuming the declaration is correct) this function is always safe to call, regardless of arg values. So may be that's what the comment should say.
There was a problem hiding this comment.
+1
i think the only thing worth calling out here is that KeBugCheckEx is callable at every irql
| /// wdk_panic::install_panic_hook(); | ||
| /// ``` | ||
| #[cfg(all(not(test), driver_model__driver_type = "UMDF"))] | ||
| pub fn install_panic_hook() { |
There was a problem hiding this comment.
Longer term we could automatically insert a call to this function in a UMDF driver through the #[driver_entry] attribute provided by the wdf crate.
There was a problem hiding this comment.
+1! there are various versions of attribute macros floating around to help with the friction around bridging the gap between c callbacks and rust functions. this is definitely one of the things we can put in the glue layer
|
|
||
| [dependencies] | ||
| wdk = "0.4.1" | ||
| wdk-panic = "0.4.1" |
There was a problem hiding this comment.
That logic works only in release-plz PRs so we can't switch to 0.5.1 in this PR. We have to wait until the next release.
Description
Previous panic handlers would initiate an infinite loop.
This PR adds custom panic handler implementations for both WDM/KMDF and UMDF Rust drivers and focuses on providing best effort information to a debugger.
TODO: Update the UMDF cargo-wdk
lib.rstemplate to include UMDF panic registration. New issue created to track this: Issue 646Verification