timer: fix oss-fuzz issue #11852 #6982
Conversation
Signed-off-by: Ismo Puustinen <ismo.puustinen@intel.com>
Signed-off-by: Ismo Puustinen <ismo.puustinen@intel.com>
source/common/event/timer_impl.cc
Outdated
| @@ -23,7 +23,17 @@ void TimerImpl::enableTimer(const std::chrono::milliseconds& d) { | |||
| event_active(&raw_event_, EV_TIMEOUT, 0); | |||
| } else { | |||
| // TODO(#4332): use duration_cast more nicely to clean up this code. | |||
| std::chrono::microseconds us = std::chrono::duration_cast<std::chrono::microseconds>(d); | |||
|
|
|||
There was a problem hiding this comment.
Should we try and follow the above TODO and try and make parts of this calculation more duration_cast based? If that's done, does this math change a bit?
There was a problem hiding this comment.
The method described in the TODO seems to work fine, and removes the need for clamping the value. Using that now!
| @@ -0,0 +1,2 @@ | |||
| static_resources { clusters { name: " " connect_timeout { nanos: 4 } hosts { | |||
There was a problem hiding this comment.
Could you also add a unit test for this new clamping or conversion boundary behavior somewhere, e.g. https://github.com/envoyproxy/envoy/blob/master/test/common/event/dispatcher_impl_test.cc#L189? The corpus entries are useful, but they are hard to reason about when trying to understand behavior, and are subject to fuzzer rot.
There was a problem hiding this comment.
I added some tests. I refactored the timeval conversion to a separate function to make testing easier.
|
This pull request has been automatically marked as stale because it has not had activity in the last 7 days. It will be closed in 7 days if no further activity occurs. Please feel free to give a status update now, ping for review, or re-open when it's ready. Thank you for your contributions! |
stale
bot
added
the
stale
|
@ipuustin friendly ping on this one. |
stale
bot
removed
the
stale
The approach proposed in envoyproxy#4332 indeed solves the integer overflow issue (and results in nicer code). Refactor the conversion in a separate function for testing and add some basic tests. Signed-off-by: Ismo Puustinen <ismo.puustinen@intel.com>
Signed-off-by: Ismo Puustinen <ismo.puustinen@intel.com>
source/common/event/timer_impl.cc
Outdated
| @@ -18,25 +18,21 @@ TimerImpl::TimerImpl(Libevent::BasePtr& libevent, TimerCb cb) : cb_(cb) { | |||
|
|
|||
| void TimerImpl::disableTimer() { event_del(&raw_event_); } | |||
|
|
|||
| void TimerImpl::millisecondsToTimeval(timeval* tv, const std::chrono::milliseconds& d) { | |||
There was a problem hiding this comment.
Tiny nit: Envoy tends to have the order be input parameters followed by output parameters, and to use mutable ref. So, can this be TimerImpl::millisecondsToTimeval(const std::chrono::milliseconds& d, timeval& tv)?
There was a problem hiding this comment.
Sure! This is good advice regarding the code conventions.
source/common/event/timer_impl.cc
Outdated
| @@ -18,25 +18,21 @@ TimerImpl::TimerImpl(Libevent::BasePtr& libevent, TimerCb cb) : cb_(cb) { | |||
|
|
|||
| void TimerImpl::disableTimer() { event_del(&raw_event_); } | |||
|
|
|||
| void TimerImpl::millisecondsToTimeval(timeval* tv, const std::chrono::milliseconds& d) { | |||
| ASSERT(tv); | |||
source/common/event/timer_impl.h
Outdated
| @@ -22,6 +22,9 @@ class TimerImpl : public Timer, ImplBase { | |||
| void enableTimer(const std::chrono::milliseconds& d) override; | |||
| bool enabled() override; | |||
|
|
|||
| // Public for testing. | |||
| void millisecondsToTimeval(timeval* tv, const std::chrono::milliseconds& d); | |||
There was a problem hiding this comment.
Suggest putting in a utility class, e.g. TimerUtils and making static
There was a problem hiding this comment.
Ok. I was originally aiming to do "minimal" changes but a separate class makes sense.
Signed-off-by: Ismo Puustinen <ismo.puustinen@intel.com>
|
/retest |
|
🔨 rebuilding |
Description:
Fix a time conversion signed int overflow (https://bugs.chromium.org/p/oss-fuzz/issues/detail?id=11852).
Risk Level: low
Testing: fuzz