Repository: mitsuhiko/minijinja (language: rust)

Issue:
Landing Pad For Order Violations
After #707 it's now possible for objects to influence how they compare.  This lets someone implement violations to the sort order, and in fact that is very much expected to happen since the Python bindings forward this to Python.

Today the Python binding will just panic if you try to sort things that cannot be compared.

One option would be to add a feature that uses `catch_unwind` around sort calls to fail with a better error than just a panic.


Discussion / hints:

Fix patch:
diff --git a/minijinja-py/Cargo.toml b/minijinja-py/Cargo.toml
index 280206c0..1457a72b 100644
--- a/minijinja-py/Cargo.toml
+++ b/minijinja-py/Cargo.toml
@@ -9,6 +9,6 @@ name = "minijinja_py"
 crate-type = ["cdylib"]
 
 [dependencies]
-minijinja = { version = "2.7.0", path = "../minijinja", features = ["loader", "json", "urlencode", "fuel", "preserve_order", "speedups", "custom_syntax", "loop_controls"] }
+minijinja = { version = "2.7.0", path = "../minijinja", features = ["loader", "json", "urlencode", "fuel", "preserve_order", "speedups", "custom_syntax", "loop_controls", "internal_safe_search"] }
 once_cell = "1.17.0"
 pyo3 = { version = "0.23.4", features = ["extension-module", "serde", "abi3-py38"] }
diff --git a/minijinja-py/python/minijinja/__init__.py b/minijinja-py/python/minijinja/__init__.py
index 437d5672..3aca489f 100644
--- a/minijinja-py/python/minijinja/__init__.py
+++ b/minijinja-py/python/minijinja/__init__.py
@@ -11,6 +11,30 @@
 ]
 
 
+def handle_panic(orig):
+    def decorator(f):
+        from functools import wraps
+
+        @wraps(orig)
+        def protected_call(*args, **kwargs):
+            try:
+                return f(*args, **kwargs)
+            except BaseException as e:
+                if e.__class__.__name__ == "PanicException":
+                    info = _lowlevel.get_panic_info()
+                    message, loc = info or ("unknown panic", None)
+                    raise TemplateError(
+                        "panic during rendering: {} ({})".format(
+                            message, loc or "unknown location"
+                        )
+                    )
+                raise
+
+        return protected_call
+
+    return decorator
+
+
 class Environment(_lowlevel.Environment):
     """Represents a MiniJinja environment"""
 
@@ -90,6 +114,14 @@ def __init__(
         self.line_statement_prefix = line_statement_prefix
         self.line_comment_prefix = line_comment_prefix
 
+    @handle_panic(_lowlevel.Environment.render_str)
+    def render_str(self, *args, **kwargs):
+        return super().render_str(*args, **kwargs)
+
+    @handle_panic(_lowlevel.Environment.eval_expr)
+    def eval_expr(self, *args, **kwargs):
+        return super().eval_expr(*args, **kwargs)
+
 
 DEFAULT_ENVIRONMENT = Environment()
 
@@ -185,3 +217,6 @@ def __str__(self):
         if self._info is not None:
             return self._info.full_description
         return self.message
+
+
+del handle_panic
diff --git a/minijinja-py/src/error_support.rs b/minijinja-py/src/error_support.rs
index b670a3c5..3d3f5040 100644
--- a/minijinja-py/src/error_support.rs
+++ b/minijinja-py/src/error_support.rs
@@ -1,3 +1,4 @@
+use std::any::Any;
 use std::cell::RefCell;
 
 use minijinja::{Error, ErrorKind};
@@ -10,6 +11,7 @@ static TEMPLATE_ERROR: OnceCell<Py<PyAny>> = OnceCell::new();
 
 thread_local! {
     static STASHED_ERROR: RefCell<Option<PyErr>> = const { RefCell::new(None) };
+    static PANIC_INFO: RefCell<Option<(String, Option<String>)>> = const { RefCell::new(None) };
 }
 
 /// Provides information about a template error from the runtime.
@@ -104,3 +106,29 @@ fn make_error(err: Error) -> PyErr {
         PyErr::from_value(template_error.call1(py, args).unwrap().bind(py).clone())
     })
 }
+
+fn payload_as_str(payload: &dyn Any) -> &str {
+    if let Some(&s) = payload.downcast_ref::<&'static str>() {
+        s
+    } else if let Some(s) = payload.downcast_ref::<String>() {
+        s.as_str()
+    } else {
+        "unknown error"
+    }
+}
+
+pub(crate) fn init_panic_hook() {
+    std::panic::set_hook(Box::new(|info| {
+        let msg = payload_as_str(info.payload());
+        let loc = info.location();
+        PANIC_INFO.with(|stash| {
+            let str_loc = loc.map(|loc| format!("{}:{}", loc.file(), loc.line()));
+            *stash.borrow_mut() = Some((msg.to_string(), str_loc));
+        });
+    }));
+}
+
+#[pyfunction]
+pub(crate) fn get_panic_info() -> PyResult<Option<(String, Option<String>)>> {
+    Ok(PANIC_INFO.with(|stash| stash.borrow().clone()))
+}
diff --git a/minijinja-py/src/lib.rs b/minijinja-py/src/lib.rs
index 10cc7f24..38e386fd 100644
--- a/minijinja-py/src/lib.rs
+++ b/minijinja-py/src/lib.rs
@@ -10,5 +10,7 @@ fn _lowlevel(_py: Python, m: &Bound<'_, PyModule>) -> PyResult<()> {
     m.add_class::<environment::Environment>()?;
     m.add_class::<state::StateRef>()?;
     m.add_class::<error_support::ErrorInfo>()?;
+    m.add_function(wrap_pyfunction!(error_support::get_panic_info, m)?)?;
+    crate::error_support::init_panic_hook();
     Ok(())
 }
diff --git a/minijinja/Cargo.toml b/minijinja/Cargo.toml
index a7c08051..f2b063e2 100644
--- a/minijinja/Cargo.toml
+++ b/minijinja/Cargo.toml
@@ -60,6 +60,7 @@ key_interning = []
 internal_debug = []
 unstable_machinery = ["internal_debug"]
 unstable_machinery_serde = ["unstable_machinery", "serde/derive"]
+internal_safe_search = []
 
 [dependencies]
 aho-corasick = { version = "1.0", default-features = false, optional = true }
diff --git a/minijinja/src/filters.rs b/minijinja/src/filters.rs
index c4e36668..57345f91 100644
--- a/minijinja/src/filters.rs
+++ b/minijinja/src/filters.rs
@@ -293,7 +293,7 @@ mod builtins {
     use super::*;
 
     use crate::error::ErrorKind;
-    use crate::utils::splitn_whitespace;
+    use crate::utils::{safe_sort, splitn_whitespace};
     use crate::value::ops::{self, as_f64};
     use crate::value::{Enumerator, Kwargs, Object, ObjectRepr, ValueKind, ValueRepr};
     use std::borrow::Cow;
@@ -433,10 +433,10 @@ mod builtins {
         let mut rv: Vec<_> = ok!(v.try_iter())
             .map(|key| (key.clone(), v.get_item(&key).unwrap_or(Value::UNDEFINED)))
             .collect();
-        rv.sort_by(|a, b| {
+        safe_sort(&mut rv, |a, b| {
             let (a, b) = if by_value { (&a.1, &b.1) } else { (&a.0, &b.0) };
             cmp_helper(a, b, case_sensitive)
-        });
+        })?;
         if let Some(true) = ok!(kwargs.get("reverse")) {
             rv.reverse();
         }
@@ -865,12 +865,14 @@ mod builtins {
         .collect::<Vec<_>>();
         let case_sensitive = ok!(kwargs.get::<Option<bool>>("case_sensitive")).unwrap_or(false);
         if let Some(attr) = ok!(kwargs.get::<Option<&str>>("attribute")) {
-            items.sort_by(|a, b| match (a.get_path(attr), b.get_path(attr)) {
-                (Ok(a), Ok(b)) => cmp_helper(&a, &b, case_sensitive),
-                _ => Ordering::Equal,
-            });
+            safe_sort(&mut items, |a, b| {
+                match (a.get_path(attr), b.get_path(attr)) {
+                    (Ok(a), Ok(b)) => cmp_helper(&a, &b, case_sensitive),
+                    _ => Ordering::Equal,
+                }
+            })?;
         } else {
-            items.sort_by(|a, b| cmp_helper(a, b, case_sensitive))
+            safe_sort(&mut items, |a, b| cmp_helper(a, b, case_sensitive))?;
         }
         if let Some(true) = ok!(kwargs.get("reverse")) {
             items.reverse();
@@ -1465,11 +1467,11 @@ mod builtins {
             None => ok!(kwargs.get::<&str>("attribute")),
         };
         let mut items: Vec<Value> = ok!(value.try_iter()).collect();
-        items.sort_by(|a, b| {
+        safe_sort(&mut items, |a, b| {
             let a = a.get_path_or_default(attr, &default);
             let b = b.get_path_or_default(attr, &default);
             cmp_helper(&a, &b, case_sensitive)
-        });
+        })?;
         ok!(kwargs.assert_all_used());
 
         #[derive(Debug)]
diff --git a/minijinja/src/utils.rs b/minijinja/src/utils.rs
index b74da090..c4fae9a7 100644
--- a/minijinja/src/utils.rs
+++ b/minijinja/src/utils.rs
@@ -1,4 +1,5 @@
 use std::char::decode_utf16;
+use std::cmp::Ordering;
 use std::collections::BTreeMap;
 use std::fmt;
 use std::iter::{once, repeat};
@@ -393,6 +394,40 @@ pub fn splitn_whitespace(s: &str, maxsplits: usize) -> impl Iterator<Item = &str
     })
 }
 
+/// Because the Python crate violates our ordering guarantees by design
+/// we want to catch failed sorts in a landing pad.  This is not ideal but
+/// it at least gives us error context for when invalid search operations
+/// are taking place.
+#[cfg_attr(not(feature = "internal_safe_search"), inline)]
+pub fn safe_sort<T, F>(seq: &mut [T], f: F) -> Result<(), Error>
+where
+    F: FnMut(&T, &T) -> Ordering,
+{
+    #[cfg(feature = "internal_safe_search")]
+    {
+        if let Err(panic) = std::panic::catch_unwind(std::panic::AssertUnwindSafe(move || {
+            seq.sort_by(f);
+        })) {
+            let msg = panic
+                .downcast_ref::<&str>()
+                .copied()
+                .or_else(|| panic.downcast_ref::<String>().map(|x| x.as_str()));
+            return Err(Error::new(
+                ErrorKind::InvalidOperation,
+                format!(
+                    "failed to sort: {}",
+                    msg.unwrap_or("comparator does not implement total order")
+                ),
+            ));
+        }
+    }
+    #[cfg(not(feature = "internal_safe_search"))]
+    {
+        seq.sort_by(f);
+    }
+    Ok(())
+}
+
 #[cfg(test)]
 mod tests {
     use super::*;


Test patch:
diff --git a/minijinja-py/tests/test_basic.py b/minijinja-py/tests/test_basic.py
index b0c45003..999f6fe5 100644
--- a/minijinja-py/tests/test_basic.py
+++ b/minijinja-py/tests/test_basic.py
@@ -1,6 +1,7 @@
 import binascii
 import pytest
 import posixpath
+import random
 import types
 from functools import total_ordering
 
@@ -413,3 +414,17 @@ def __str__(self):
     env = Environment()
     rv = env.render_str("{{ values|sort|join(',') }}", values=values)
     assert rv == "-1,4,23,42"
+
+
+def test_fucked_up_object():
+    @total_ordering
+    class X:
+        __lt__ = __eq__ = lambda s, o: random.random() > 0.5
+
+    values = [X()] * 500
+    env = Environment()
+    with pytest.raises(
+        TemplateError,
+        match="invalid operation: failed to sort: user-provided comparison function does not correctly implement a total order",
+    ):
+        env.eval_expr("values|sort", values=values)