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.

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

Unchecked add of instant + duration is causing a panic. #1488

Closed
empath-nirvana opened this issue May 9, 2024 · 5 comments · Fixed by #1489
Closed

Unchecked add of instant + duration is causing a panic. #1488

empath-nirvana opened this issue May 9, 2024 · 5 comments · Fixed by #1489
Labels
bug Something isn't working

Comments

@empath-nirvana
Copy link

empath-nirvana commented May 9, 2024

Current and expected behavior

https://github.com/kube-rs/kube/blob/0.91.0/kube-runtime/src/controller/mod.rs#L475

I'm getting a panic here. I'm sure it's because we've accidentally queued up some long retry duration, but it shouldn't panic even if you do that. Digging through the code, there's actually quite a few unchecked additions of times in the code.

Possible solution

use checked adds.

Additional context

No response

Environment

We're using the latest version of EKS 1.29, but i don't think it's relevant.

Configuration and features

No response

Affected crates

kube-runtime

Would you like to work on fixing this bug?

None

@empath-nirvana empath-nirvana added the bug Something isn't working label May 9, 2024
@clux
Copy link
Member

clux commented May 9, 2024

oh, wow. i wasn't aware that could panic. thanks for the report.

guess we need to move to https://doc.rust-lang.org/std/time/struct.Instant.html#method.checked_add everywhere as you say, but that does imply injecting a default instant if it fails. maybe a lower number, defaulting to now?


as an aside, i am curious about why this actually panics in practice. like, is this from bad monotonicity conditions with low numbers, or is there a problem with user requeue / debounce numbers that causes them to grow beyond the bounds of systemtime?

clux added a commit that referenced this issue May 9, 2024
Fixes #1488

Signed-off-by: clux <sszynrae@gmail.com>
@Dav1dde
Copy link
Member

Dav1dde commented May 9, 2024

Are you running a debug build? Overflow checks are not enabled by default in release builds.

Edit: nvm, didn't realize this is different for Instants, TIL

@empath-nirvana
Copy link
Author

We found and fixed our bug that triggered this.. we had some code that sometimes generated a negative retry time, which we were casting to an unsigned int, which created a very large number.

@SOF3
Copy link
Contributor

SOF3 commented May 10, 2024

as an aside, i am curious about why this actually panics in practice. like, is this from bad monotonicity conditions with low numbers, or is there a problem with user requeue / debounce numbers that causes them to grow beyond the bounds of systemtime?

isn't this just an integer overflow of the system monotonic counter?

I think it is reasonable to panic in that case, since passing such a large value is practically a bug. If this duration originated from user input, it is the input component that should have validated this.

@clux
Copy link
Member

clux commented May 10, 2024

We found and fixed our bug that triggered this.. we had some code that sometimes generated a negative retry time

Thanks for confirming this.

isn't this just an integer overflow

yeah, i was reading too much into all the scary notes in the instant docs about monotonicity. confirmed above as near-max int overflow, so just merging the fix in #1489.

clux added a commit that referenced this issue May 10, 2024
* Fix potentially panicing unchecked duration adds in runtime

Fixes #1488

Signed-off-by: clux <sszynrae@gmail.com>

* use far_future as a default for instant overflow

Signed-off-by: clux <sszynrae@gmail.com>

---------

Signed-off-by: clux <sszynrae@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants