Skip to content

Commit

Permalink
Fix an integer overflow on division/abs and neg (#347)
Browse files Browse the repository at this point in the history
  • Loading branch information
mitsuhiko authored Sep 16, 2023
1 parent b9cc11a commit ed9f6bc
Show file tree
Hide file tree
Showing 5 changed files with 50 additions and 7 deletions.
5 changes: 5 additions & 0 deletions CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,11 @@

All notable changes to MiniJinja are documented here.

## 1.0.8

- Fixed a few overflow panics: dividing integers with an overflow and
related overflows in the `abs` and `neg` filter. #347

## 1.0.7

- Added support for `keep_trailing_newlines` which allows you to disable
Expand Down
11 changes: 9 additions & 2 deletions minijinja/Cargo.toml
Original file line number Diff line number Diff line change
Expand Up @@ -17,7 +17,14 @@ features = ["loader", "json", "urlencode", "custom_syntax", "fuel"]
rustdoc-args = ["--cfg", "docsrs", "--html-in-header", "doc-header.html"]

[features]
default = ["builtins", "debug", "deserialization", "macros", "multi_template", "adjacent_loop_items"]
default = [
"builtins",
"debug",
"deserialization",
"macros",
"multi_template",
"adjacent_loop_items",
]

# API features
preserve_order = ["indexmap"]
Expand Down Expand Up @@ -60,7 +67,7 @@ unicode-ident = { version = "1.0.5", optional = true }
unicase = { version = "2.6.0", optional = true }

[dev-dependencies]
insta = { version = "1.26.0", features = ["glob", "serde"] }
insta = { version = "1.31.0", features = ["glob", "serde"] }
serde = { version = "1.0.130", features = ["derive"] }
serde_json = "1.0.68"
similar-asserts = "1.4.2"
11 changes: 9 additions & 2 deletions minijinja/src/filters.rs
Original file line number Diff line number Diff line change
Expand Up @@ -552,8 +552,15 @@ mod builtins {
#[cfg_attr(docsrs, doc(cfg(feature = "builtins")))]
pub fn abs(value: Value) -> Result<Value, Error> {
match value.0 {
ValueRepr::I64(x) => Ok(Value::from(x.abs())),
ValueRepr::I128(x) => Ok(Value::from(x.0.abs())),
ValueRepr::I64(x) => match x.checked_abs() {
Some(rv) => Ok(Value::from(rv)),
None => Ok(Value::from((x as i128).abs())), // this cannot overflow
},
ValueRepr::I128(x) => {
x.0.checked_abs()
.map(Value::from)
.ok_or_else(|| Error::new(ErrorKind::InvalidOperation, "overflow on abs"))
}
ValueRepr::F64(x) => Ok(Value::from(x.abs())),
_ => Err(Error::new(
ErrorKind::InvalidOperation,
Expand Down
20 changes: 18 additions & 2 deletions minijinja/src/value/ops.rs
Original file line number Diff line number Diff line change
Expand Up @@ -204,7 +204,9 @@ pub fn int_div(lhs: &Value, rhs: &Value) -> Result<Value, Error> {
match coerce(lhs, rhs) {
Some(CoerceResult::I128(a, b)) => {
if b != 0 {
Ok(int_as_value(a.div_euclid(b)))
a.checked_div_euclid(b)
.ok_or_else(|| failed_op("//", lhs, rhs))
.map(int_as_value)
} else {
Err(failed_op("//", lhs, rhs))
}
Expand Down Expand Up @@ -235,7 +237,9 @@ pub fn neg(val: &Value) -> Result<Value, Error> {
ValueRepr::F64(x) => Ok((-x).into()),
_ => {
if let Ok(x) = i128::try_from(val.clone()) {
Ok(int_as_value(-x))
x.checked_mul(-1)
.ok_or_else(|| Error::new(ErrorKind::InvalidOperation, "overflow"))
.map(int_as_value)
} else {
Err(Error::from(ErrorKind::InvalidOperation))
}
Expand Down Expand Up @@ -283,6 +287,12 @@ mod tests {

use similar_asserts::assert_eq;

#[test]
fn test_neg() {
let err = neg(&Value::from(i128::MIN)).unwrap_err();
assert_eq!(err.to_string(), "invalid operation: overflow");
}

#[test]
fn test_adding() {
let err = add(&Value::from("a"), &Value::from(42)).unwrap_err();
Expand Down Expand Up @@ -339,6 +349,12 @@ mod tests {
div(&Value::from(100), &Value::from(2)).unwrap(),
Value::from(50.0)
);

let err = int_div(&Value::from(i128::MIN), &Value::from(-1i128)).unwrap_err();
assert_eq!(
err.to_string(),
"invalid operation: unable to calculate -170141183460469231731687303715884105728 // -1"
);
}

#[test]
Expand Down
10 changes: 9 additions & 1 deletion minijinja/tests/test_filters.rs
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,7 @@ use minijinja::value::Value;
use minijinja::{args, Environment};
use similar_asserts::assert_eq;

use minijinja::filters::indent;
use minijinja::filters::{abs, indent};

#[test]
fn test_filter_with_non() {
Expand Down Expand Up @@ -77,3 +77,11 @@ fn test_indent_with_all_indented() {
String::from(" test\n test1\n \n test2")
);
}

#[test]
fn test_abs_overflow() {
let ok = abs(Value::from(i64::MIN)).unwrap();
assert_eq!(ok, Value::from(-(i64::MIN as i128)));
let err = abs(Value::from(i128::MIN)).unwrap_err();
assert_eq!(err.to_string(), "invalid operation: overflow on abs");
}

0 comments on commit ed9f6bc

Please sign in to comment.