Merge pull request #1058 from schungx/master

Fix panic in sorting.

Author: schungx

CHANGELOG.md

@@ -9,13 +9,19 @@ Breaking Changes
 
 * `stdweb` support is removed. The feature flag `stdweb` is also removed. Use `wasm-bindgen` instead.
 
+Bug fixes
+---------
+
+* (Fuzzing) Fixed panic when using `sort` on an array with a comparer function that does not implement a [total order](https://en.wikipedia.org/wiki/Total_order).
 
 Enhancements
 ------------
 
 * The optimizer now optimizes constant object map property accesses (thanks [`@phsym`](https://github.com/phsym) [#1050](https://github.com/rhaiscript/rhai/pull/1050)).
+* Optimization is added to turn `if cond1 { if cond2 { ... } }` into `if cond1 && cond2 { ... }`.
 * The method `sort` for arrays is also aliased to `sort_by`.
 * The methods `sort_desc`, `order`, `order_by` and `order_desc` are added to arrays.
+* An exception is now thrown when attempting to use `sort` on an array containing unsupported element types.
 
 
 Version 1.23.6

src/optimizer.rs

@@ -15,7 +15,7 @@ use crate::func::hashing::get_hasher;
 use crate::tokenizer::Token;
 use crate::{
     calc_fn_hash, calc_fn_hash_full, Dynamic, Engine, FnArgsVec, FnPtr, ImmutableString, Position,
-    Scope, AST,
+    Scope, StaticVec, AST,
 };
 #[cfg(feature = "no_std")]
 use std::prelude::v1::*;
@@ -498,6 +498,37 @@ fn optimize_stmt(stmt: &mut Stmt, state: &mut OptimizerState, preserve_result: b
                 }
             }
         }
+        // if expr1 { if expr2 { ... } } -> if expr1 && expr2 { ... }
+        Stmt::If(x, pos)
+            if x.branch.is_empty()
+                && x.body.len() == 1
+                && matches!(&x.body.as_ref()[0], Stmt::If(x2, ..) if x2.branch.is_empty()) =>
+        {
+            let Stmt::If(mut x2, ..) = x.body.as_mut()[0].take() else {
+                unreachable!()
+            };
+
+            state.set_dirty();
+            let body = x2.body.take_statements();
+            *x.body.statements_mut() =
+                optimize_stmt_block(body, state, preserve_result, true, false);
+
+            let mut expr2 = x2.expr.take();
+            optimize_expr(&mut expr2, state, false);
+
+            if let Expr::And(ref mut v, ..) = x.expr {
+                v.push(expr2);
+            } else {
+                let mut expr1 = x.expr.take();
+                optimize_expr(&mut expr1, state, false);
+
+                let mut v = StaticVec::new_const();
+                v.push(expr1);
+                v.push(expr2);
+
+                x.expr = Expr::And(v.into(), *pos);
+            }
+        }
         // if expr { if_block } else { else_block }
         Stmt::If(x, ..) => {
             let FlowControl { expr, body, branch } = &mut **x;

src/packages/array_basic.rs

@@ -1514,6 +1514,8 @@ pub mod array_functions {
     }
     /// Sort the array based on applying the `comparer` function.
     ///
+    /// The `comparer` function must implement a [total order](https://en.wikipedia.org/wiki/Total_order).
+    ///
     /// # Function Parameters
     ///
     /// * `element1`: copy of the current array element to compare
@@ -1534,6 +1536,11 @@ pub mod array_functions {
     ///
     /// Any other return value type will yield unpredictable order.
     ///
+    /// # Errors
+    ///
+    /// If the `comparer` function does not implement a [total order](https://en.wikipedia.org/wiki/Total_order),
+    /// an error is returned.
+    ///
     /// # Example
     ///
     /// ```rhai
@@ -1544,31 +1551,38 @@ pub mod array_functions {
     ///
     /// print(x);       // prints "[10, 9, 8, 7, 6, 5, 4, 3, 2, 1]"
     /// ```
-    #[rhai_fn(name = "sort", name = "sort_by")]
-    pub fn sort_by(ctx: NativeCallContext, array: &mut Array, comparer: FnPtr) {
+    #[rhai_fn(name = "sort", name = "sort_by", return_raw)]
+    pub fn sort_by(ctx: NativeCallContext, array: &mut Array, comparer: FnPtr) -> RhaiResultOf<()> {
         if array.len() <= 1 {
-            return;
+            return Ok(());
         }
 
-        array.sort_by(|x, y| {
-            comparer
-                .call_raw(&ctx, None, [x.clone(), y.clone()])
-                .ok()
-                .and_then(|v| {
-                    v.as_int()
-                        .or_else(|_| v.as_bool().map(|v| if v { -1 } else { 1 }))
-                        .ok()
-                })
-                .map_or_else(
-                    || x.type_id().cmp(&y.type_id()),
-                    |v| match v {
-                        v if v > 0 => Ordering::Greater,
-                        v if v < 0 => Ordering::Less,
-                        0 => Ordering::Equal,
-                        _ => unreachable!("v is {}", v),
-                    },
-                )
-        });
+        // `slice::sort_by` may panic if the comparer function does not implement a total order.
+        // Catch this instead.
+        std::panic::catch_unwind(std::panic::AssertUnwindSafe(|| {
+            array.sort_by(|x, y| {
+                comparer
+                    .call_raw(&ctx, None, [x.clone(), y.clone()])
+                    .ok()
+                    .and_then(|v| {
+                        v.as_int()
+                            .or_else(|_| v.as_bool().map(|v| if v { -1 } else { 1 }))
+                            .ok()
+                    })
+                    .map_or_else(
+                        || x.type_id().cmp(&y.type_id()),
+                        |v| match v {
+                            v if v > 0 => Ordering::Greater,
+                            v if v < 0 => Ordering::Less,
+                            0 => Ordering::Equal,
+                            _ => unreachable!("v is {}", v),
+                        },
+                    )
+            });
+        }))
+        .map_err(|_| {
+            ERR::ErrorRuntime("error in comparer for sorting".into(), Position::NONE).into()
+        })
     }
     /// Sort the array.
     ///
@@ -1603,7 +1617,7 @@ pub mod array_functions {
 
         if array.iter().any(|a| a.type_id() != type_id) {
             return Err(ERR::ErrorFunctionNotFound(
-                "sort() cannot be called with elements of different types".into(),
+                "elements of different types cannot be sorted".into(),
                 Position::NONE,
             )
             .into());
@@ -1663,7 +1677,11 @@ pub mod array_functions {
             return Ok(());
         }
 
-        Ok(())
+        Err(ERR::ErrorFunctionNotFound(
+            format!("elements of {} cannot be sorted", array[0].type_name()).into(),
+            Position::NONE,
+        )
+        .into())
     }
     /// Sort the array in descending order.
     ///
@@ -1698,7 +1716,7 @@ pub mod array_functions {
 
         if array.iter().any(|a| a.type_id() != type_id) {
             return Err(ERR::ErrorFunctionNotFound(
-                "sort_desc() cannot be called with elements of different types".into(),
+                "elements of different types cannot be sorted".into(),
                 Position::NONE,
             )
             .into());
@@ -1782,10 +1800,16 @@ pub mod array_functions {
             return Ok(());
         }
 
-        Ok(())
+        Err(ERR::ErrorFunctionNotFound(
+            format!("elements of {} cannot be sorted", array[0].type_name()).into(),
+            Position::NONE,
+        )
+        .into())
     }
     /// Sort the array based on applying the `comparer` function and return it as a new array.
     ///
+    /// The `comparer` function must implement a [total order](https://en.wikipedia.org/wiki/Total_order).
+    ///
     /// # Function Parameters
     ///
     /// * `element1`: copy of the current array element to compare
@@ -1806,6 +1830,11 @@ pub mod array_functions {
     ///
     /// Any other return value type will yield unpredictable order.
     ///
+    /// # Errors
+    ///
+    /// If the `comparer` function does not implement a [total order](https://en.wikipedia.org/wiki/Total_order),
+    /// an error is returned.
+    ///
     /// # Example
     ///
     /// ```rhai
@@ -1816,10 +1845,15 @@ pub mod array_functions {
     ///
     /// print(y);       // prints "[10, 9, 8, 7, 6, 5, 4, 3, 2, 1]"
     /// ```
-    pub fn order_by(ctx: NativeCallContext, array: &mut Array, comparer: FnPtr) -> Array {
+    #[rhai_fn(name = "order", name = "order_by", return_raw)]
+    pub fn order_by(
+        ctx: NativeCallContext,
+        array: &mut Array,
+        comparer: FnPtr,
+    ) -> RhaiResultOf<Array> {
         let mut array = array.clone();
-        sort_by(ctx, &mut array, comparer);
-        array
+        sort_by(ctx, &mut array, comparer)?;
+        Ok(array)
     }
     /// Sort the array and return it as a new array.
     ///