fix: arithmetic overflow panic #252
Conversation
| Value::Integer(lhv) => { | ||
| let rhv = rhs.try_into_i64().map_err(|_| err())?; | ||
| i64::wrapping_mul(lhv, rhv).into() | ||
| } |
There was a problem hiding this comment.
Is overflow the general pattern we use arithmetic? Or returning errors? Just figured it's worth writing that in https://github.com/vectordotdev/vector/blob/master/docs/DEVELOPING.md (or adding a similar file to VRL).
There was a problem hiding this comment.
I'm open to suggestions, but I figured it's worth at least making debug builds match what release builds currently do. Returning errors would require all multiplication to be fallible, which could be pretty painful to use. Another option might be to convert the result to an f64 if it would overflow, but again that would change the type definition and probably make it pretty painful to use in practice. I'll add a comment related to this. It seems a bit too specific for DEVELOPING.md
There was a problem hiding this comment.
Although you bring up a good point, this should probably be consistent across all arithmetic. I'll modify those too. (addition, subtraction, etc.)
There was a problem hiding this comment.
Yeah, agreed, we seem to be using silent overflow for release builds, generally, so we can just codify that rule until we find a reason to change it.
Would it make sense to introduce the integer_arithmetic clippy lint?
There was a problem hiding this comment.
I added the integer_arithmetic lint to just the arithmetic.rs file, which is where it's important. There are quite a few false positives outside of that file.
fuchsnj
changed the title
| #[allow(clippy::integer_arithmetic)] | ||
| let mut value = BytesMut::with_capacity(lhs.len() + rhs.len()); |
There was a problem hiding this comment.
I wonder if we wouldn't actually want this to panic. The behavior otherwise seems like it'd be strange.
There was a problem hiding this comment.
Today you could never get into a situation where this would overflow. If either of these values are large enough to cause an overflow, that means you would have already allocated several million terrabytes.
In general though, I'd like to add some kind of allocation limit in VRL for #242
There was a problem hiding this comment.
True, true, that makes me think even more, then, that it should panic rather than overflow 😄
closes #238
Multiplication, addition and subtraction overflows could panic when running in a debug build. This is fixed to always use wrapping arithmetic.