Skip to content

Commit

Permalink
fix(console): prevent panics if subscriber reports out-of-order times (
Browse files Browse the repository at this point in the history
…#295)

Changes duration_since and unchecked subtractions to assume
`Duration::ZERO` (via `Default`) if the alternative would be a crash.

If the subscriber reports nonsensical data this can result in negative
`Duration`s in console's calculations, leading to panics. This has
already happened inadvertently. All changed code relates directly to
received protobuf data whose validity isn't guaranteed.

Relates to #266 (crash) and #289 (fix for root cause)

---

There are a handful of other unwraps/expects I'm not so sure about.
These ones looked like pretty clear-cut cases where panicking is
possible, undesirable, and clamping to zero is a reasonable response.
  • Loading branch information
thombles authored Mar 7, 2022
1 parent b4e6b6a commit 80d7f42
Show file tree
Hide file tree
Showing 3 changed files with 23 additions and 17 deletions.
14 changes: 8 additions & 6 deletions tokio-console/src/state/async_ops.rs
Original file line number Diff line number Diff line change
Expand Up @@ -211,7 +211,7 @@ impl AsyncOpsState {
.stats
.dropped_at
.map(|d| {
let dropped_for = now.duration_since(d).unwrap();
let dropped_for = now.duration_since(d).unwrap_or_default();
retain_for > dropped_for
})
.unwrap_or(true)
Expand Down Expand Up @@ -247,14 +247,15 @@ impl AsyncOp {
pub(crate) fn total(&self, since: SystemTime) -> Duration {
self.stats
.total
.unwrap_or_else(|| since.duration_since(self.stats.created_at).unwrap())
.or_else(|| since.duration_since(self.stats.created_at).ok())
.unwrap_or_default()
}

pub(crate) fn busy(&self, since: SystemTime) -> Duration {
if let (Some(last_poll_started), None) =
(self.stats.last_poll_started, self.stats.last_poll_ended)
{
let current_time_in_poll = since.duration_since(last_poll_started).unwrap();
let current_time_in_poll = since.duration_since(last_poll_started).unwrap_or_default();
return self.stats.busy + current_time_in_poll;
}
self.stats.busy
Expand All @@ -263,7 +264,8 @@ impl AsyncOp {
pub(crate) fn idle(&self, since: SystemTime) -> Duration {
self.stats
.idle
.unwrap_or_else(|| self.total(since) - self.busy(since))
.or_else(|| self.total(since).checked_sub(self.busy(since)))
.unwrap_or_default()
}

pub(crate) fn total_polls(&self) -> u64 {
Expand Down Expand Up @@ -309,11 +311,11 @@ impl AsyncOpStats {
.unwrap();

let dropped_at: Option<SystemTime> = pb.dropped_at.map(|v| v.try_into().unwrap());
let total = dropped_at.map(|d| d.duration_since(created_at).unwrap());
let total = dropped_at.map(|d| d.duration_since(created_at).unwrap_or_default());

let poll_stats = pb.poll_stats.expect("task should have poll stats");
let busy = poll_stats.busy_time.map(pb_duration).unwrap_or_default();
let idle = total.map(|total| total - busy);
let idle = total.map(|total| total.checked_sub(busy).unwrap_or_default());
let formatted_attributes = Attribute::make_formatted(styles, &mut attributes);
let task_id = pb.task_id.map(|id| task_ids.id_for(id.id));
let task_id_str = strings.string(
Expand Down
12 changes: 7 additions & 5 deletions tokio-console/src/state/resources.rs
Original file line number Diff line number Diff line change
Expand Up @@ -246,7 +246,7 @@ impl ResourcesState {
.stats
.dropped_at
.map(|d| {
let dropped_for = now.duration_since(d).unwrap();
let dropped_for = now.duration_since(d).unwrap_or_default();
retain_for > dropped_for
})
.unwrap_or(true)
Expand Down Expand Up @@ -296,9 +296,11 @@ impl Resource {
}

pub(crate) fn total(&self, since: SystemTime) -> Duration {
self.stats
.total
.unwrap_or_else(|| since.duration_since(self.stats.created_at).unwrap())
self.stats.total.unwrap_or_else(|| {
since
.duration_since(self.stats.created_at)
.unwrap_or_default()
})
}

pub(crate) fn dropped(&self) -> bool {
Expand Down Expand Up @@ -338,7 +340,7 @@ impl ResourceStats {
.try_into()
.unwrap();
let dropped_at: Option<SystemTime> = pb.dropped_at.map(|v| v.try_into().unwrap());
let total = dropped_at.map(|d| d.duration_since(created_at).unwrap());
let total = dropped_at.map(|d| d.duration_since(created_at).unwrap_or_default());

Self {
created_at,
Expand Down
14 changes: 8 additions & 6 deletions tokio-console/src/state/tasks.rs
Original file line number Diff line number Diff line change
Expand Up @@ -206,7 +206,7 @@ impl TasksState {
task.stats
.dropped_at
.map(|d| {
let dropped_for = now.duration_since(d).unwrap();
let dropped_for = now.duration_since(d).unwrap_or_default();
retain_for > dropped_for
})
.unwrap_or(true)
Expand Down Expand Up @@ -281,15 +281,16 @@ impl Task {
pub(crate) fn total(&self, since: SystemTime) -> Duration {
self.stats
.total
.unwrap_or_else(|| since.duration_since(self.stats.created_at).unwrap())
.or_else(|| since.duration_since(self.stats.created_at).ok())
.unwrap_or_default()
}

pub(crate) fn busy(&self, since: SystemTime) -> Duration {
if let (Some(last_poll_started), None) =
(self.stats.last_poll_started, self.stats.last_poll_ended)
{
// in this case the task is being polled at the moment
let current_time_in_poll = since.duration_since(last_poll_started).unwrap();
let current_time_in_poll = since.duration_since(last_poll_started).unwrap_or_default();
return self.stats.busy + current_time_in_poll;
}
self.stats.busy
Expand All @@ -298,7 +299,8 @@ impl Task {
pub(crate) fn idle(&self, since: SystemTime) -> Duration {
self.stats
.idle
.unwrap_or_else(|| self.total(since) - self.busy(since))
.or_else(|| self.total(since).checked_sub(self.busy(since)))
.unwrap_or_default()
}

/// Returns the total number of times the task has been polled.
Expand Down Expand Up @@ -388,11 +390,11 @@ impl From<proto::tasks::Stats> for TaskStats {
.unwrap();

let dropped_at: Option<SystemTime> = pb.dropped_at.map(|v| v.try_into().unwrap());
let total = dropped_at.map(|d| d.duration_since(created_at).unwrap());
let total = dropped_at.map(|d| d.duration_since(created_at).unwrap_or_default());

let poll_stats = pb.poll_stats.expect("task should have poll stats");
let busy = poll_stats.busy_time.map(pb_duration).unwrap_or_default();
let idle = total.map(|total| total - busy);
let idle = total.map(|total| total.checked_sub(busy).unwrap_or_default());
Self {
total,
idle,
Expand Down

0 comments on commit 80d7f42

Please sign in to comment.