From 75801bdb0e559c73d3f5ea99f1ce1429a425cd1b Mon Sep 17 00:00:00 2001 From: har7an <99636919+har7an@users.noreply.github.com> Date: Sun, 23 Oct 2022 13:14:24 +0000 Subject: [PATCH] plugins: Improve error handling on plugin version mismatch (#1838) * server/tab: Don't panic in `Pane::render` and do not crash the application on failure to receive a render update from plugins any longer. Instead, will print a simple string with a hint to check the application logs, where a more thorough error indication can be found. * utils/errors: re-export `anyhow::Error` to create ad-hoc errors with custom error types, without having to wrap them into a `context()` before to turn the into anyhow errors. * plugins: Check plugin version on startup and terminate execution with a descriptive error message in case the plugin version is incompatible with the version of zellij being run. * server/wasm_vm: Add plugin path in version error so the user knows which plugin to look at in case they're using custom plugins. * server/wasm_vm: Check plugin version for equality Previously we would accept cases where the plugin version was newer than the zellij version, which doesn't make a lot of sense. * server/wasm_vm: Prettier error handling in call to `wasmer::Function::call` in case a plugin version mismatch can occur. * tile: Install custom panic handler that will print the panic message to a plugins stdout and then call a panic handler on the host that turns it into a real application-level panic. * tile: Catch errors in event deserialization and turn them into proper panics. These errors are symptomatic of an uncaught plugin version mismatch, for example when developing from main and compiling zellij/the plugins from source. Normal users should never get to see this error. * utils/errors: Improve output in `to_stdout` for anyhow errors. The default anyhow error formatting of `{:?}` is already very good, and we just made it worse by trying to invent our own formatting. * tile: Reword plugin mismatch error message * zellij: Apply rustfmt * changelog: Add PR #1838 Improve error handling on plugin version mismatch. * server/wasm_vm: Rephrase error in passive voice --- CHANGELOG.md | 1 + Cargo.lock | 1 + zellij-server/Cargo.toml | 1 + zellij-server/src/panes/plugin_pane.rs | 27 +++-- zellij-server/src/panes/terminal_pane.rs | 11 +- zellij-server/src/tab/mod.rs | 2 +- zellij-server/src/ui/pane_contents_and_ui.rs | 5 +- zellij-server/src/wasm_vm.rs | 114 ++++++++++++++++--- zellij-tile/src/lib.rs | 31 ++++- zellij-tile/src/prelude.rs | 2 + zellij-tile/src/shim.rs | 17 ++- zellij-utils/src/errors.rs | 7 +- 12 files changed, 179 insertions(+), 40 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 2f9da8906a..ce447db75b 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -35,6 +35,7 @@ The format is based on [Keep a Changelog](https://keepachangelog.com/en/1.0.0/) * feat: allow defining tab cwd in layouts (https://github.com/zellij-org/zellij/pull/1828) * debugging: Remove calls to `unwrap` from plugin WASM VM (https://github.com/zellij-org/zellij/pull/1827) * debugging: Improve error handling in `server/route` (https://github.com/zellij-org/zellij/pull/1808) +* debugging: Detect plugin version mismatches (https://github.com/zellij-org/zellij/pull/1838) ## [0.31.4] - 2022-09-09 * Terminal compatibility: improve vttest compliance (https://github.com/zellij-org/zellij/pull/1671) diff --git a/Cargo.lock b/Cargo.lock index 7ba98d4345..2fe99fddd4 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -3405,6 +3405,7 @@ dependencies = [ "highway", "insta", "log", + "semver", "serde_json", "sixel-image", "sixel-tokenizer", diff --git a/zellij-server/Cargo.toml b/zellij-server/Cargo.toml index 5b8035a89a..bf6d0cd254 100644 --- a/zellij-server/Cargo.toml +++ b/zellij-server/Cargo.toml @@ -31,6 +31,7 @@ sixel-tokenizer = "0.1.0" sixel-image = "0.1.0" arrayvec = "0.7.2" uuid = { version = "0.8.2", features = ["serde", "v4"] } +semver = "0.11.0" [dev-dependencies] insta = "1.6.0" diff --git a/zellij-server/src/panes/plugin_pane.rs b/zellij-server/src/panes/plugin_pane.rs index e872d614a2..5709d7fee3 100644 --- a/zellij-server/src/panes/plugin_pane.rs +++ b/zellij-server/src/panes/plugin_pane.rs @@ -15,6 +15,7 @@ use zellij_utils::shared::ansi_len; use zellij_utils::{ channels::SenderWithContext, data::{Event, InputMode, Mouse, PaletteColor}, + errors::prelude::*, pane_size::{Dimension, PaneGeom}, shared::make_terminal_title, }; @@ -137,12 +138,16 @@ impl Pane for PluginPane { fn render( &mut self, client_id: Option, - ) -> Option<(Vec, Option, Vec)> { + ) -> Result, Option, Vec)>> { // this is a bit of a hack but works in a pinch - client_id?; - let client_id = client_id.unwrap(); + let client_id = match client_id { + Some(id) => id, + None => return Ok(None), + }; // if self.should_render { if true { + let err_context = || format!("failed to render plugin panes for client {client_id}"); + // while checking should_render rather than rendering each pane every time // is more performant, it causes some problems when the pane to the left should be // rendered and has wide characters (eg. Chinese characters or emoji) @@ -158,12 +163,16 @@ impl Pane for PluginPane { self.get_content_rows(), self.get_content_columns(), )) - .unwrap(); + .to_anyhow() + .with_context(err_context)?; self.should_render = false; + // This is where we receive the text to render from the plugins. let contents = buf_rx .recv() - .expect("Failed to receive reply from plugin. Please check the logs"); + .with_context(err_context) + .to_log() + .unwrap_or("No output from plugin received. See logs".to_string()); for (index, line) in contents.lines().enumerate() { let actual_len = ansi_len(line); let line_to_print = if actual_len > self.get_content_columns() { @@ -185,7 +194,7 @@ impl Pane for PluginPane { self.get_content_x() + 1, line_to_print, ) - .unwrap(); // goto row/col and reset styles + .with_context(err_context)?; // goto row/col and reset styles let line_len = line_to_print.len(); if line_len < self.get_content_columns() { // pad line @@ -206,15 +215,15 @@ impl Pane for PluginPane { y + line_index + 1, x + 1 ) - .unwrap(); // goto row/col and reset styles + .with_context(err_context)?; // goto row/col and reset styles for _col_index in 0..self.get_content_columns() { vte_output.push(' '); } } } - Some((vec![], Some(vte_output), vec![])) // TODO: PluginPanes should have their own grid so that we can return the non-serialized TerminalCharacters and have them participate in the render buffer + Ok(Some((vec![], Some(vte_output), vec![]))) // TODO: PluginPanes should have their own grid so that we can return the non-serialized TerminalCharacters and have them participate in the render buffer } else { - None + Ok(None) } } fn render_frame( diff --git a/zellij-server/src/panes/terminal_pane.rs b/zellij-server/src/panes/terminal_pane.rs index 766cfaa2bd..8d0651ad2a 100644 --- a/zellij-server/src/panes/terminal_pane.rs +++ b/zellij-server/src/panes/terminal_pane.rs @@ -17,6 +17,7 @@ use zellij_utils::input::command::RunCommand; use zellij_utils::pane_size::Offset; use zellij_utils::{ data::{InputMode, Palette, PaletteColor, Style}, + errors::prelude::*, pane_size::SizeInPixels, pane_size::{Dimension, PaneGeom}, position::Position, @@ -272,7 +273,7 @@ impl Pane for TerminalPane { fn render( &mut self, _client_id: Option, - ) -> Option<(Vec, Option, Vec)> { + ) -> Result, Option, Vec)>> { if self.should_render() { let mut raw_vte_output = String::new(); let content_x = self.get_content_x(); @@ -332,9 +333,13 @@ impl Pane for TerminalPane { self.grid.ring_bell = false; } self.set_should_render(false); - Some((character_chunks, Some(raw_vte_output), sixel_image_chunks)) + Ok(Some(( + character_chunks, + Some(raw_vte_output), + sixel_image_chunks, + ))) } else { - None + Ok(None) } } fn render_frame( diff --git a/zellij-server/src/tab/mod.rs b/zellij-server/src/tab/mod.rs index da2fcda607..c63cac89c9 100644 --- a/zellij-server/src/tab/mod.rs +++ b/zellij-server/src/tab/mod.rs @@ -142,7 +142,7 @@ pub trait Pane { fn render( &mut self, client_id: Option, - ) -> Option<(Vec, Option, Vec)>; // TODO: better + ) -> Result, Option, Vec)>>; // TODO: better fn render_frame( &mut self, client_id: ClientId, diff --git a/zellij-server/src/ui/pane_contents_and_ui.rs b/zellij-server/src/ui/pane_contents_and_ui.rs index 779a690941..4db9feb4ba 100644 --- a/zellij-server/src/ui/pane_contents_and_ui.rs +++ b/zellij-server/src/ui/pane_contents_and_ui.rs @@ -45,7 +45,8 @@ impl<'a> PaneContentsAndUi<'a> { &mut self, clients: impl Iterator, ) { - if let Some((character_chunks, raw_vte_output, sixel_image_chunks)) = self.pane.render(None) + if let Some((character_chunks, raw_vte_output, sixel_image_chunks)) = + self.pane.render(None).unwrap() { let clients: Vec = clients.collect(); self.output.add_character_chunks_to_multiple_clients( @@ -73,7 +74,7 @@ impl<'a> PaneContentsAndUi<'a> { } pub fn render_pane_contents_for_client(&mut self, client_id: ClientId) { if let Some((character_chunks, raw_vte_output, sixel_image_chunks)) = - self.pane.render(Some(client_id)) + self.pane.render(Some(client_id)).unwrap() { self.output .add_character_chunks_to_client(client_id, character_chunks, self.z_index); diff --git a/zellij-server/src/wasm_vm.rs b/zellij-server/src/wasm_vm.rs index 6a013a277e..4fea6adb0a 100644 --- a/zellij-server/src/wasm_vm.rs +++ b/zellij-server/src/wasm_vm.rs @@ -1,9 +1,10 @@ use highway::{HighwayHash, PortableHash}; use log::{debug, info, warn}; +use semver::Version; use serde::{de::DeserializeOwned, Serialize}; use std::{ collections::{HashMap, HashSet}, - fs, + fmt, fs, path::{Path, PathBuf}, process, str::FromStr, @@ -39,12 +40,40 @@ use zellij_utils::{ serde, }; -// String to add as error context when we fail to call the `load` function on a plugin. -// The usual cause of an error in this call is that the plugin versions don't match the zellij -// version. -const PLUGINS_OUT_OF_DATE: &str = - "If you're seeing this error the most likely cause is that your plugin versions -don't match your current zellij version. +/// Custom error for plugin version mismatch. +/// +/// This is thrown when, during starting a plugin, it is detected that the plugin version doesn't +/// match the zellij version. This is treated as a fatal error and leads to instantaneous +/// termination. +#[derive(Debug)] +pub struct VersionMismatchError { + zellij_version: String, + plugin_version: String, + plugin_path: PathBuf, +} + +impl std::error::Error for VersionMismatchError {} + +impl VersionMismatchError { + pub fn new(zellij_version: &str, plugin_version: &str, plugin_path: &PathBuf) -> Self { + VersionMismatchError { + zellij_version: zellij_version.to_owned(), + plugin_version: plugin_version.to_owned(), + plugin_path: plugin_path.to_owned(), + } + } +} + +impl fmt::Display for VersionMismatchError { + fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result { + write!( + f, + "If you're seeing this error the plugin versions don't match the current +zellij version. Detected versions: + +- Plugin version: {} +- Zellij version: {} +- Offending plugin: {} If you're a user: Please contact the distributor of your zellij version and report this error @@ -57,7 +86,13 @@ If you're a developer: A possible fix for this error is to remove all contents of the 'PLUGIN DIR' folder from the output of the `zellij setup --check` command. -"; +", + self.plugin_version, + self.zellij_version, + self.plugin_path.display() + ) + } +} #[derive(Clone, Debug)] pub(crate) enum PluginInstruction { @@ -161,9 +196,9 @@ pub(crate) fn wasm_thread_main( PluginInstruction::Update(pid, cid, event) => { let err_context = || { if *DEBUG_MODE.get().unwrap_or(&true) { - format!("failed to update plugin with event: {event:#?}") + format!("failed to update plugin state with event: {event:#?}") } else { - "failed to update plugin".to_string() + "failed to update plugin state".to_string() } }; @@ -187,10 +222,19 @@ pub(crate) fn wasm_thread_main( .get_function("update") .with_context(err_context)?; wasi_write_object(&plugin_env.wasi_env, &event); - update - .call(&[]) - .with_context(err_context) - .context(PLUGINS_OUT_OF_DATE)?; + update.call(&[]).or_else::(|e| { + match e.downcast::() { + Ok(_) => panic!( + "{}", + anyError::new(VersionMismatchError::new( + VERSION, + "Unavailable", + &plugin_env.plugin.path + )) + ), + Err(e) => Err(e).with_context(err_context), + } + })?; } } drop(bus.senders.send_to_screen(ScreenInstruction::Render)); @@ -386,6 +430,37 @@ fn start_plugin( let zellij = zellij_exports(store, &plugin_env); let instance = Instance::new(&module, &zellij.chain_back(wasi)).with_context(err_context)?; + // Check plugin version + let plugin_version_func = match instance.exports.get_function("plugin_version") { + Ok(val) => val, + Err(_) => panic!( + "{}", + anyError::new(VersionMismatchError::new( + VERSION, + "Unavailable", + &plugin_env.plugin.path + )) + ), + }; + plugin_version_func.call(&[]).with_context(err_context)?; + let plugin_version_str = wasi_read_string(&plugin_env.wasi_env); + let plugin_version = Version::parse(&plugin_version_str) + .context("failed to parse plugin version") + .with_context(err_context)?; + let zellij_version = Version::parse(VERSION) + .context("failed to parse zellij version") + .with_context(err_context)?; + if plugin_version != zellij_version { + panic!( + "{}", + anyError::new(VersionMismatchError::new( + VERSION, + &plugin_version_str, + &plugin_env.plugin.path + )) + ); + } + Ok((instance, plugin_env)) } @@ -425,6 +500,7 @@ pub(crate) fn zellij_exports(store: &Store, plugin_env: &PluginEnv) -> ImportObj host_switch_tab_to, host_set_timeout, host_exec_cmd, + host_report_panic, } } @@ -546,6 +622,16 @@ fn host_exec_cmd(plugin_env: &PluginEnv) { .unwrap(); } +// Custom panic handler for plugins. +// +// This is called when a panic occurs in a plugin. Since most panics will likely originate in the +// code trying to deserialize an `Event` upon a plugin state update, we read some panic message, +// formatted as string from the plugin. +fn host_report_panic(plugin_env: &PluginEnv) { + let msg = wasi_read_string(&plugin_env.wasi_env); + panic!("{}", msg); +} + // Helper Functions --------------------------------------------------------------------------------------------------- pub fn wasi_read_string(wasi_env: &WasiEnv) -> String { diff --git a/zellij-tile/src/lib.rs b/zellij-tile/src/lib.rs index 85a5bfe1fc..00dc60a96c 100644 --- a/zellij-tile/src/lib.rs +++ b/zellij-tile/src/lib.rs @@ -10,6 +10,18 @@ pub trait ZellijPlugin { fn render(&mut self, rows: usize, cols: usize) {} } +pub const PLUGIN_MISMATCH: &str = + "An error occured in a plugin while receiving an Event from zellij. This means +that the plugins aren't compatible with the current zellij version. + +The most likely explanation for this is that you're running either a +self-compiled zellij or plugin version. Please make sure that, while developing, +you also rebuild the plugins in order to pick up changes to the plugin code. + +Please refer to the documentation for further information: + https://github.com/zellij-org/zellij/blob/main/CONTRIBUTING.md#building +"; + #[macro_export] macro_rules! register_plugin { ($t:ty) => { @@ -18,6 +30,11 @@ macro_rules! register_plugin { } fn main() { + // Register custom panic handler + std::panic::set_hook(Box::new(|info| { + report_panic(info); + })); + STATE.with(|state| { state.borrow_mut().load(); }); @@ -25,10 +42,13 @@ macro_rules! register_plugin { #[no_mangle] pub fn update() { + let object = $crate::shim::object_from_stdin() + .context($crate::PLUGIN_MISMATCH) + .to_stdout() + .unwrap(); + STATE.with(|state| { - state - .borrow_mut() - .update($crate::shim::object_from_stdin().unwrap()); + state.borrow_mut().update(object); }); } @@ -38,5 +58,10 @@ macro_rules! register_plugin { state.borrow_mut().render(rows as usize, cols as usize); }); } + + #[no_mangle] + pub fn plugin_version() { + println!("{}", $crate::prelude::VERSION); + } }; } diff --git a/zellij-tile/src/prelude.rs b/zellij-tile/src/prelude.rs index 8190206673..21d1317f68 100644 --- a/zellij-tile/src/prelude.rs +++ b/zellij-tile/src/prelude.rs @@ -1,4 +1,6 @@ pub use crate::shim::*; pub use crate::*; +pub use zellij_utils::consts::VERSION; pub use zellij_utils::data::*; +pub use zellij_utils::errors::prelude::*; pub use zellij_utils::input::actions; diff --git a/zellij-tile/src/shim.rs b/zellij-tile/src/shim.rs index 99920e744a..0aa8d3e0ec 100644 --- a/zellij-tile/src/shim.rs +++ b/zellij-tile/src/shim.rs @@ -1,6 +1,7 @@ use serde::{de::DeserializeOwned, Serialize}; use std::{io, path::Path}; use zellij_utils::data::*; +use zellij_utils::errors::prelude::*; // Subscription Handling @@ -50,13 +51,22 @@ pub fn exec_cmd(cmd: &[&str]) { unsafe { host_exec_cmd() }; } +pub fn report_panic(info: &std::panic::PanicInfo) { + println!(""); + println!("A panic occured in a plugin"); + println!("{:#?}", info); + unsafe { host_report_panic() }; +} + // Internal Functions #[doc(hidden)] -pub fn object_from_stdin() -> Result { +pub fn object_from_stdin() -> Result { + let err_context = || "failed to deserialize object from stdin".to_string(); + let mut json = String::new(); - io::stdin().read_line(&mut json).unwrap(); - serde_json::from_str(&json) + io::stdin().read_line(&mut json).with_context(err_context)?; + serde_json::from_str(&json).with_context(err_context) } #[doc(hidden)] @@ -75,4 +85,5 @@ extern "C" { fn host_switch_tab_to(tab_idx: u32); fn host_set_timeout(secs: f64); fn host_exec_cmd(); + fn host_report_panic(); } diff --git a/zellij-utils/src/errors.rs b/zellij-utils/src/errors.rs index 0f7db131c2..7ec02b9a53 100644 --- a/zellij-utils/src/errors.rs +++ b/zellij-utils/src/errors.rs @@ -27,6 +27,7 @@ pub mod prelude { pub use anyhow::anyhow; pub use anyhow::bail; pub use anyhow::Context; + pub use anyhow::Error as anyError; pub use anyhow::Result; } @@ -111,11 +112,7 @@ pub trait LoggableError: Sized { impl LoggableError for anyhow::Result { fn print_error(self, fun: F) -> Self { if let Err(ref err) = self { - let mut msg = format!("ERROR: {}", err); - for cause in err.chain().skip(1) { - msg = format!("{msg}\nbecause: {cause}"); - } - fun(&msg); + fun(&format!("{:?}", err)); } self }