Skip to content

Commit

Permalink
fix: arithmetic overflow panic (#252)
Browse files Browse the repository at this point in the history
  • Loading branch information
fuchsnj committed May 24, 2023
1 parent c7c95b8 commit 1ff642d
Show file tree
Hide file tree
Showing 8 changed files with 36 additions and 11 deletions.
1 change: 1 addition & 0 deletions CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,7 @@
## unreleased
- added the `timezone` argument to the `format_timestamp` vrl function.
- removed feature flags for each individual VRL function.
- fixed a panic when arithmetic overflows. It now always wraps (only in debug builds).

- `ingress_upstreaminfo` log format has been added to `parse_nginx_log` function (https://github.com/vectordotdev/vrl/pull/193)

Expand Down
6 changes: 2 additions & 4 deletions lib/fuzz/src/main.rs
Original file line number Diff line number Diff line change
Expand Up @@ -14,10 +14,8 @@ use vrl::value::Secrets;
fn main() {
fuzz!(|data: &[u8]| {
if let Ok(src) = std::str::from_utf8(data) {
if src.contains('\\') || src.contains('*') {
// skipping known issues
// - invalid escapes
// - overflow from multiplication
if src.contains('\\') {
// skipping known issues with invalid escapes
return;
}
if src.len() > 10_000 {
Expand Down
3 changes: 3 additions & 0 deletions lib/tests/tests/expressions/arithmetic/addition/overflow.vrl
Original file line number Diff line number Diff line change
@@ -0,0 +1,3 @@
# result: -9223372036854775808

9223372036854775807+1
Original file line number Diff line number Diff line change
@@ -0,0 +1,3 @@
# result: 3578153274754795185

444111111111*1111111111111111
3 changes: 3 additions & 0 deletions lib/tests/tests/expressions/arithmetic/remainder/overflow.vrl
Original file line number Diff line number Diff line change
@@ -0,0 +1,3 @@
# result: 0

mod(-9223372036854775808,-1)
Original file line number Diff line number Diff line change
@@ -0,0 +1,3 @@
# result: 9223372036854775807

-9223372036854775808 - 1
1 change: 1 addition & 0 deletions src/compiler/expression/op.rs
Original file line number Diff line number Diff line change
Expand Up @@ -106,6 +106,7 @@ impl Expression for Op {
let lhs = self.lhs.resolve(ctx)?;
let rhs = self.rhs.resolve(ctx)?;

// Arithmetic that can overflow should wrap
match self.opcode {
Mul => lhs.try_mul(rhs),
Div => lhs.try_div(rhs),
Expand Down
27 changes: 20 additions & 7 deletions src/compiler/value/arithmetic.rs
Original file line number Diff line number Diff line change
@@ -1,3 +1,5 @@
#![deny(clippy::integer_arithmetic)]

use std::collections::BTreeMap;

use crate::value::Value;
Expand Down Expand Up @@ -73,7 +75,10 @@ impl VrlValueArithmetic for Value {
Value::Integer(lhv) if rhs.is_float() => {
Value::from_f64_or_zero(lhv as f64 * rhs.try_float()?)
}
Value::Integer(lhv) => (lhv * rhs.try_into_i64().map_err(|_| err())?).into(),
Value::Integer(lhv) => {
let rhv = rhs.try_into_i64().map_err(|_| err())?;
i64::wrapping_mul(lhv, rhv).into()
}
Value::Float(lhv) => (lhv * rhs.try_into_f64().map_err(|_| err())?).into(),
Value::Bytes(lhv) if rhs.is_integer() => {
Bytes::from(lhv.repeat(as_usize(rhs.try_integer()?))).into()
Expand Down Expand Up @@ -107,18 +112,20 @@ impl VrlValueArithmetic for Value {
fn try_add(self, rhs: Self) -> Result<Self, ValueError> {
let value = match (self, rhs) {
(Value::Integer(lhs), Value::Float(rhs)) => Value::from_f64_or_zero(lhs as f64 + *rhs),
(Value::Integer(lhs), rhs) => (lhs
+ rhs
(Value::Integer(lhs), rhs) => {
let rhv = rhs
.try_into_i64()
.map_err(|_| ValueError::Add(Kind::integer(), rhs.kind()))?)
.into(),
.map_err(|_| ValueError::Add(Kind::integer(), rhs.kind()))?;
i64::wrapping_add(lhs, rhv).into()
}
(Value::Float(lhs), rhs) => (lhs
+ rhs
.try_into_f64()
.map_err(|_| ValueError::Add(Kind::float(), rhs.kind()))?)
.into(),
(lhs @ Value::Bytes(_), Value::Null) => lhs,
(Value::Bytes(lhs), Value::Bytes(rhs)) => {
#[allow(clippy::integer_arithmetic)]
let mut value = BytesMut::with_capacity(lhs.len() + rhs.len());
value.put(lhs);
value.put(rhs);
Expand All @@ -139,7 +146,10 @@ impl VrlValueArithmetic for Value {
Value::Integer(lhv) if rhs.is_float() => {
Value::from_f64_or_zero(lhv as f64 - rhs.try_float()?)
}
Value::Integer(lhv) => (lhv - rhs.try_into_i64().map_err(|_| err())?).into(),
Value::Integer(lhv) => {
let rhv = rhs.try_into_i64().map_err(|_| err())?;
i64::wrapping_sub(lhv, rhv).into()
}
Value::Float(lhv) => (lhv - rhs.try_into_f64().map_err(|_| err())?).into(),
_ => return Err(err()),
};
Expand Down Expand Up @@ -198,7 +208,10 @@ impl VrlValueArithmetic for Value {
Value::Integer(lhv) if rhs.is_float() => {
Value::from_f64_or_zero(lhv as f64 % rhs.try_float()?)
}
Value::Integer(lhv) => (lhv % rhs.try_into_i64().map_err(|_| err())?).into(),
Value::Integer(lhv) => {
let rhv = rhs.try_into_i64().map_err(|_| err())?;
i64::wrapping_rem(lhv, rhv).into()
}
Value::Float(lhv) => (lhv % rhs.try_into_f64().map_err(|_| err())?).into(),
_ => return Err(err()),
};
Expand Down

0 comments on commit 1ff642d

Please sign in to comment.