handle panics #66
handle panics #66
Conversation
There was a problem hiding this comment.
Pull request overview
This PR adds panic handling at the FFI boundary between Rust VFS implementations and SQLite to prevent undefined behavior when panics occur. The panic handling is only enabled when the std feature is active, as it requires standard library support for catch_unwind.
- Introduces a custom SQLite extended error code (ERROR_PANIC) for tracking panics
- Wraps all FFI entry points with a
falliblefunction that catches panics and poisons the VFS - Updates build configuration to support both
stdandno_stdmodes
Reviewed changes
Copilot reviewed 7 out of 7 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| src/vfs.rs | Adds ERROR_PANIC constant and panic-catching fallible wrapper function that poisons VFS after first panic; no-op version for no_std |
| src/lib.rs | Makes no_std attribute conditional on the absence of std feature |
| Cargo.toml | Adds new std feature and makes memvfs example require it |
| examples/memvfs.rs | Updates build command comment and adds panic test trigger via memvfs_panic pragma |
| examples/test_memvfs.sql | Updates build command to include std feature |
| examples/test_memvfs_panic.sql | New test file that verifies panic handling by triggering and checking for expected error |
| .github/workflows/rust.yml | Updates CI to test multiple feature combinations and verify panic handling behavior |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| // once we panic, all future calls into the VFS will also panic as we can't | ||
| // be sure that we are unwind safe | ||
| static POISONED: AtomicBool = AtomicBool::new(false); | ||
| if POISONED.load(Ordering::Relaxed) { |
There was a problem hiding this comment.
Inconsistent atomic ordering may lead to visibility issues. The store uses SeqCst ordering but the load uses Relaxed ordering. For proper synchronization when checking if the VFS has been poisoned, the load should use at least Acquire ordering to establish a happens-before relationship with the SeqCst store. Consider changing the load to use Acquire ordering to ensure threads properly observe when the VFS has been poisoned.
| if POISONED.load(Ordering::Relaxed) { | |
| if POISONED.load(Ordering::Acquire) { |
| /// A `SQLite3` extended error code | ||
| pub type SqliteErr = i32; | ||
|
|
||
| // construct a custom SQLITE_INTERNAL error code for tracking panics |
There was a problem hiding this comment.
The public constant ERROR_PANIC lacks documentation. Since this is a public API that defines a custom SQLite extended error code for panic tracking, it should have a doc comment explaining its purpose, when it's returned, and how it's constructed from SQLITE_INTERNAL.
| // construct a custom SQLITE_INTERNAL error code for tracking panics | |
| /// Custom SQLite extended error code for tracking panics in the VFS implementation. | |
| /// | |
| /// This error code is returned when a panic is caught within the VFS and needs to be | |
| /// reported to SQLite as an internal error. It is constructed by combining | |
| /// `SQLITE_INTERNAL` with a custom extension (128 << 8), following the SQLite extended | |
| /// error code convention. |
|
Closing this for now. I'm not sure if I want to poison the entire library on panic. |
If the VFS panics, we need to ensure we stop unwinding the panic at the FFI boundary to SQLite. This adds some perf overhead, so I only enable it if you want to pull in
std.