Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

to_float and infinity result can crash vector #1107

Open
dhable opened this issue Nov 1, 2024 · 2 comments · May be fixed by #1108
Open

to_float and infinity result can crash vector #1107

dhable opened this issue Nov 1, 2024 · 2 comments · May be fixed by #1108

Comments

@dhable
Copy link

dhable commented Nov 1, 2024

We ran into an issue where parsing a string with to_float exceeds the bounds of f64::MAX, which returns a non-error infinity value.

$ str_value = "61687855628556285562855628556285562855628556285562855628556285562855628556285562855628556285562855628556285562855628556285562855628556285562855628556285562855628556285562855628556285562855628556285562855628556285562855628556285562855628556285562855628556285562855628556285562855628556285562855628556285562855628556285562855628556285562"
"61687855628556285562855628556285562855628556285562855628556285562855628556285562855628556285562855628556285562855628556285562855628556285562855628556285562855628556285562855628556285562855628556285562855628556285562855628556285562855628556285562855628556285562855628556285562855628556285562855628556285562855628556285562855628556285562"

$ parsed_value, err = to_float(str_value)
inf

$ err
null

Since err was null, it's might seem like it's safe to perform operations on this value but subtraction causes vector to panic and crash. This is because f64::INFINITY - f64::INFINITY == f64::NAN in rust.

$ parsed_value + parsed_value
inf

$ parsed_value + 1
inf

$ parsed_value - 1
inf

$ 1 - parsed_value
-inf

$ parsed_value - parsed_value
thread 'main' panicked at /Users/dan.hable/.cargo/registry/src/index.crates.io-6f17d22bba15001f/ordered-float-4.2.2/src/lib.rs:1331:37:
Subtraction resulted in NaN: FloatIsNan
note: run with `RUST_BACKTRACE=1` environment variable to display a backtrace

I can't find anything in the VRL language, besides using to_string to compare with "inf", that would allow me to detect this situation and avoid operations on infinite values. IMHO, I would have expected an error result instead of infinity, similar to how to_int behaves.

$ parsed_value, err = to_int(str_value)
"function call error for \"to_int\" at (20:29): Invalid integer \"61687855628556285562855628556285562855628556285562855628556285562855628556285562855628556285562855628556285562855628556285562855628556285562855628556285562855628556285562855628556285562855628556285562855628556285562855628556285562855628556285562855628556285562855628556285562855628556285562855628556285562855628556285562855628556285562\": number too large to fit in target type"
dhable added a commit to dhable/vrl that referenced this issue Nov 1, 2024
@dhable
fix(convert): return error when conversion exceeds f64 bounds
2ede709 
Prior to this commit, string values that would exceed the f64 bounds would
return `f64::INFINITY`. This leaks a type into the VRL scripts that could
cause a panic without support to check for the value.

Fixes vectordotdev#1107
@dhable dhable linked a pull request Nov 1, 2024 that will close this issue
Open
11 tasks
@pront
Copy link
Contributor

pront commented Nov 1, 2024

Thanks @dhable, I will take a look. Need to think a bit about the breaking behavior of this change.

@pront
Copy link
Contributor

pront commented Nov 4, 2024 •
edited
Loading

I think the proposed (breaking) change makes sense. See playground examples. We can make the check even stricter by using is_normal.

@pront pront mentioned this issue Nov 11, 2024
Open
dhable added a commit to dhable/vrl that referenced this issue Nov 12, 2024
@dhable
fix(convert): return error when conversion exceeds f64 bounds
6e046fa 
Prior to this commit, string values that would exceed the f64 bounds would
return `f64::INFINITY`. This leaks a type into the VRL scripts that could
cause a panic without support to check for the value.

Fixes vectordotdev#1107
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

fix(convert): return error when conversion exceeds f64 bounds dhable/vrl
2 participants
@dhable @pront