nushell icon indicating copy to clipboard operation
nushell copied to clipboard

Panic when using the `%` character

Open brianloveswords opened this issue 2 years ago • 9 comments

Describe the bug

I'm getting panics when using shell commands that include % symbol. I've included minimal repro steps before.

How to reproduce

  1. type ls a and hit enter. (note: does not matter if a exists or not)
  2. type ls a%

Expected behavior

Not panic

Screenshots

image

Configuration

key value
version 0.64.0
branch
commit_hash
build_os macos-aarch64
build_target aarch64-apple-darwin
rust_version rustc 1.61.0
cargo_version cargo 1.61.0 (a028ae42f 2022-04-29)
pkg_version 0.64.0
build_time 2022-06-15 02:39:17 +00:00
build_rust_channel release
features database, dataframe, default, trash, which, zip
installed_plugins

Additional context

No response

brianloveswords avatar Jul 05 '22 17:07 brianloveswords

On the latest main for macos-aarch64, I'm not seeing the crash:

❯ ls a%
Error: nu::shell::directory_not_found (link)

  × Directory not found
   ╭─[entry #16:1:1]
 1 │ ls a%
   ·    ─┬
   ·     ╰── directory not found
   ╰────

Can you try again with the latest main? Or, the 0.65.0 release should be out today if you want to wait and try that

sophiajt avatar Jul 05 '22 17:07 sophiajt

Thanks for looking into this so quickly @jntrnr! I'll try again later today on either main or 0.65.0 if it's out by the time I have a chance to look more into this.

brianloveswords avatar Jul 05 '22 18:07 brianloveswords

I just tried on 0.65.0 and it panic'd, so I tried again on main and I still get it: image

here's the full stack backtrace if it's useful
   0:        0x105e757f4 - <std::sys_common::backtrace::_print::DisplayBacktrace as core::fmt::Display>::fmt::h1fcf5822ec0595d6
   1:        0x105e8eac0 - core::fmt::write::hd4312e875444daaf
   2:        0x105e5f748 - std::io::Write::write_fmt::h6fe61d5a2c45611b
   3:        0x105e65110 - std::panicking::default_hook::{{closure}}::h33762d2c242ef766
   4:        0x105e64dd4 - std::panicking::default_hook::h6d8535317e31e739
   5:        0x1045c4b58 - <alloc::boxed::Box<F,A> as core::ops::function::Fn<Args>>::call::h175bef0f9c41933f
                               at /private/tmp/rust-20220617-27679-di3d4y/rustc-1.61.0-src/library/alloc/src/boxed.rs:1875:9
   6:        0x1045e3f18 - nu::main::{{closure}}::haaaabbf39ec6ef65
                               at /Users/brian/src/nushell/src/main.rs:45:9
   7:        0x105e656d8 - std::panicking::rust_panic_with_hook::hbed5b5f05ae7b6c2
   8:        0x105e75afc - std::panicking::begin_panic_handler::{{closure}}::he6e992d2f2d44613
   9:        0x105e75908 - std::sys_common::backtrace::__rust_end_short_backtrace::h163559e3b58ec18c
  10:        0x105e65224 - _rust_begin_unwind
  11:        0x105ebf89c - core::panicking::panic_fmt::h20603f0cb36d804e
  12:        0x105e9b3f0 - core::str::slice_error_fail_rt::ha37c525d5097fa9e
  13:        0x105e98278 - core::ops::function::FnOnce::call_once::h9514d32793d8597d
  14:        0x105e98318 - core::intrinsics::const_eval_select::h2b329c4f8ee7b37a
  15:        0x105ebfa48 - core::str::slice_error_fail::h425786704163e588
  16:        0x10570b000 - core::str::traits::<impl core::slice::index::SliceIndex<str> for core::ops::range::RangeFrom<usize>>::index::h0e8dd320b77e65a6
                               at /private/tmp/rust-20220617-27679-di3d4y/rustc-1.61.0-src/library/core/src/str/traits.rs:370:21
  17:        0x105709fb4 - core::str::traits::<impl core::ops::index::Index<I> for str>::index::h70f6a96c9e0316b8
                               at /private/tmp/rust-20220617-27679-di3d4y/rustc-1.61.0-src/library/core/src/str/traits.rs:65:9
  18:        0x1054f5c28 - <alloc::string::String as core::ops::index::Index<core::ops::range::RangeFrom<usize>>>::index::hfb713cfa8be6752ae07d5
                               at /Users/brian/.cargo/registry/src/github.com-1ecc6299db9ec823/reedline-0.8.0/src/hinter/default.rs:27:21
  20:        0x1054a2b40 - core::option::Option<T>::map_or_else::h569fd9d99d3ea505
                               at /private/tmp/rust-20220617-27679-di3d4y/rustc-1.61.0-src/library/core/src/option.rs:1008:24
  21:        0x10549a640 - <reedline::hinter::default::DefaultHinter as reedline::hinter::Hinter>::handle::h833232f21d447f6b
                               at /Users/brian/.cargo/registry/src/github.com-1ecc6299db9ec823/reedline-0.8.0/src/hinter/default.rs:22:13
  22:        0x1054bf2e4 - reedline::engine::Reedline::buffer_paint::{{closure}}::hb7a428125aecf3de
                               at /Users/brian/.cargo/registry/src/github.com-1ecc6299db9ec823/reedline-0.8.0/src/engine.rs:1356:17
  23:        0x1054a2c44 - core::option::Option<T>::map_or_else::h681d061d7a91e14b
                               at /private/tmp/rust-20220617-27679-di3d4y/rustc-1.61.0-src/library/core/src/option.rs:1008:24
  24:        0x1054bef0c - reedline::engine::Reedline::buffer_paint::h1edf180579affe9b
                               at /Users/brian/.cargo/registry/src/github.com-1ecc6299db9ec823/reedline-0.8.0/src/engine.rs:1355:13
  25:        0x1054bd2d8 - reedline::engine::Reedline::repaint::hf6dd7131f0a12dba
                               at /Users/brian/.cargo/registry/src/github.com-1ecc6299db9ec823/reedline-0.8.0/src/engine.rs:1163:13
  26:        0x1054b8fdc - reedline::engine::Reedline::read_line_helper::hbd8413d216f99849
                               at /Users/brian/.cargo/registry/src/github.com-1ecc6299db9ec823/reedline-0.8.0/src/engine.rs:532:29
  27:        0x1054b8310 - reedline::engine::Reedline::read_line::h42ec59886ff179aa
                               at /Users/brian/.cargo/registry/src/github.com-1ecc6299db9ec823/reedline-0.8.0/src/engine.rs:419:22
  28:        0x10533efec - nu_cli::repl::evaluate_repl::h360af58f6337364a
                               at /Users/brian/src/nushell/crates/nu-cli/src/repl.rs:311:21
  29:        0x1045bef08 - nu::main::he0308937fe186cda
                               at /Users/brian/src/nushell/src/main.rs:295:31
  30:        0x1045d171c - core::ops::function::FnOnce::call_once::he4ceec195679f0d9
                               at /private/tmp/rust-20220617-27679-di3d4y/rustc-1.61.0-src/library/core/src/ops/function.rs:227:5
  31:        0x1045d4bcc - std::sys_common::backtrace::__rust_begin_short_backtrace::ha17ff4874ba1b584
                               at /private/tmp/rust-20220617-27679-di3d4y/rustc-1.61.0-src/library/std/src/sys_common/backtrace.rs:122:18
  32:        0x1045ec3a8 - std::rt::lang_start::{{closure}}::hb75019315c8b9f1b
                               at /private/tmp/rust-20220617-27679-di3d4y/rustc-1.61.0-src/library/std/src/rt.rs:145:18
  33:        0x105e5a840 - std::rt::lang_start_internal::h933a358395d5a923
  34:        0x1045ec370 - std::rt::lang_start::hd9118bb89348dfed
                               at /private/tmp/rust-20220617-27679-di3d4y/rustc-1.61.0-src/library/std/src/rt.rs:144:17
  35:        0x1045c2c3c - _main

Let me know what other information might be helpful!

brianloveswords avatar Jul 06 '22 00:07 brianloveswords

Thanks for the backtrace! I'm unable to reproduce on Linux x64 (Nu v0.65).

Given the presence of the Reedline hinter in the backtrace, I wonder if there's a string in your history that it's having trouble with.

rgwood avatar Jul 06 '22 00:07 rgwood

Something with the history was a good lead, I ended up resetting everything back to scratch and couldn't reproduce it, so I diffed my config with the default and remembered I have history_file_format: "sqlite" set. That seems to be required for reproduction, and I can reproduce it with an empty history using the steps at the top of this issue.

brianloveswords avatar Jul 06 '22 00:07 brianloveswords

The sqlite history doesn't escape % chars for the queries which leads them to be interpreted like a '*' glob for purposes of the history search:

https://github.com/nushell/reedline/blob/117f13b005052acaa21e959755e56cb25a898ce1/src/history/sqlite_backed.rs#L304-L312

I thought this would only lead to inaccurate results and could be fixed later, but I guess the issue is that the hinter gets resuts from a query: https://github.com/nushell/reedline/blob/117f13b005052acaa21e959755e56cb25a898ce1/src/hinter/default.rs#L21-L23

which then causes the crash because the hint is not actually a correct result

Sorry for this

phiresky avatar Jul 06 '22 14:07 phiresky

I guess the solution is to fix the escaping: For both command_line_like and cwd_prefix use X LIKE Y ESCAPE '\' and add \ before all _ and % and \ in the query.

phiresky avatar Jul 06 '22 14:07 phiresky

Something like this patch (untested):

diff --git a/src/history/sqlite_backed.rs b/src/history/sqlite_backed.rs
index 73d91be..9dc7530 100644
--- a/src/history/sqlite_backed.rs
+++ b/src/history/sqlite_backed.rs
@@ -179,6 +179,9 @@ fn map_sqlite_err(err: rusqlite::Error) -> ReedlineError {
 
 type BoxedNamedParams<'a> = Vec<(&'static str, Box<dyn ToSql + 'a>)>;
 
+fn like_escape(s: String) -> String {
+    regex::Regex::new("[%_\\]").unwrap().replace_all(s, "\\$0")
+}
 impl SqliteBackedHistory {
     /// Creates a new history with an associated history file.
     ///
@@ -305,10 +308,16 @@ impl SqliteBackedHistory {
             // TODO: escape %
             let command_line_like = match command_line {
                 CommandLineSearch::Exact(e) => e.to_string(),
-                CommandLineSearch::Prefix(prefix) => format!("{prefix}%"),
-                CommandLineSearch::Substring(cont) => format!("%{cont}%"),
+                CommandLineSearch::Prefix(prefix) => {
+                    let prefix = like_escape(prefix);
+                    format!("{prefix}%")
+                },
+                CommandLineSearch::Substring(cont) => {
+                    let cont = like_escape(cont);
+                    format!("%{cont}%")
+                },
             };
-            wheres.push("command_line like :command_line");
+            wheres.push("command_line like :command_line escape '\\'");
             params.push((":command_line", Box::new(command_line_like)));
         }
 
@@ -325,8 +334,9 @@ impl SqliteBackedHistory {
             params.push((":cwd", Box::new(cwd_exact)));
         }
         if let Some(cwd_prefix) = &query.filter.cwd_prefix {
-            wheres.push("cwd like :cwd_like");
-            let cwd_like = format!("{cwd_prefix}%");
+            wheres.push("cwd like :cwd_like escape '\\'");
+            let cwd_prefix = like_escape(cwd_prefix);
+            let cwd_like = format!("{escaped_cwd_prefix}%");
             params.push((":cwd_like", Box::new(cwd_like)));
         }
         if let Some(exit_successful) = query.filter.exit_successful {

phiresky avatar Jul 06 '22 14:07 phiresky

Any update on this panic? It's a really big show-stopper for using the sqlite history.

lukexor avatar Aug 31 '22 15:08 lukexor

I can't reproduce this with SQLite history enabled 🤔

image

key value
version 0.72.1
branch main
commit_hash 6295b205450bde26ed62f08cc825005c2267d90d
build_os linux-x86_64
build_target x86_64-unknown-linux-gnu
rust_version rustc 1.65.0 (897e37553 2022-11-02)
rust_channel 1.65.0-x86_64-unknown-linux-gnu
cargo_version cargo 1.65.0 (4bc8f24d3 2022-10-20)
pkg_version 0.72.1
build_time 2022-12-03 17:02:29 -08:00
build_rust_channel debug
features database, default, trash, which, zip
installed_plugins

rgwood avatar Dec 08 '22 03:12 rgwood

Can't reproduce panic anymore, get's properly inserted into the history, recalled or used in a autosuggestion hint. (both for filebacked and sqlite)

With 0.82.0

sholderbach avatar Jul 09 '23 19:07 sholderbach