From 98152a7a2383bdfe5087bd3c964887a75d77dd73 Mon Sep 17 00:00:00 2001 From: Andreas Reich Date: Fri, 13 Dec 2024 09:20:16 +0100 Subject: [PATCH] Fix out of bounds access in data density graph (#8441) Regressed this somewhat recently during perf optimizations in the data_density_graph: it was possible to get into an out of bounds access when zooming in. Cleaned it up a bit overall and fixed a subtle bug for when there's only fractional buckets filled in the density graph --- .../re_time_panel/src/data_density_graph.rs | 34 +++++++++++++------ 1 file changed, 24 insertions(+), 10 deletions(-) diff --git a/crates/viewer/re_time_panel/src/data_density_graph.rs b/crates/viewer/re_time_panel/src/data_density_graph.rs index 89a7487c99b5..866ae385861f 100644 --- a/crates/viewer/re_time_panel/src/data_density_graph.rs +++ b/crates/viewer/re_time_panel/src/data_density_graph.rs @@ -147,6 +147,10 @@ impl DensityGraph { pub fn add_range(&mut self, (min_x, max_x): (f32, f32), count: f32) { debug_assert!(min_x <= max_x); + if max_x < self.min_x || self.max_x < min_x { + return; + } + if min_x == max_x { let center_x = lerp(min_x..=max_x, 0.5); self.add_point(center_x, count); @@ -162,29 +166,39 @@ impl DensityGraph { // We then want to add to the buckets [3, 4, 5, 6], // but not in equal amounts. - let first_bucket_factor = 1.0 - (min_bucket - min_bucket.floor()); - let num_full_buckets = 1.0 + max_bucket.floor() - min_bucket.ceil(); - let last_bucket_factor = 1.0 - (max_bucket.ceil() - max_bucket); + let min_full_bucket = min_bucket.ceil(); + let first_bucket = min_bucket.floor(); + let max_full_bucket = max_bucket.floor(); + let last_bucket = max_bucket.ceil(); + let first_bucket_factor = 1.0 - (min_bucket - first_bucket); + let num_full_buckets = 1.0 + max_full_bucket - min_full_bucket; + let last_bucket_factor = 1.0 - (last_bucket - max_bucket); let count_per_bucket = count / (first_bucket_factor + num_full_buckets + last_bucket_factor); + // For filling self.buckets, we need to account for min_bucket/max_bucket being out of range! + // (everything before & beyond can be seen as a "virtual" bucket that we can't fill) + // first bucket, partially filled: - if let Ok(i) = usize::try_from(min_bucket.floor() as i64) { + if let Ok(i) = usize::try_from(first_bucket as i64) { if let Some(bucket) = self.buckets.get_mut(i) { *bucket += first_bucket_factor * count_per_bucket; } } // full buckets: - let min_full_bucket_idx = (min_bucket.ceil() as i64).at_least(0) as usize; - let max_full_bucket_idx = - (max_bucket.floor() as i64).at_most(self.buckets.len() as i64 - 1) as usize; - for bucket in &mut self.buckets[min_full_bucket_idx..=max_full_bucket_idx] { - *bucket += count_per_bucket; + if min_full_bucket != max_full_bucket { + let min_full_bucket_idx = + (min_full_bucket as i64).clamp(0, self.buckets.len() as i64 - 1) as usize; + let max_full_bucket_idx = + (max_full_bucket as i64).clamp(0, self.buckets.len() as i64 - 1) as usize; + for bucket in &mut self.buckets[min_full_bucket_idx..=max_full_bucket_idx] { + *bucket += count_per_bucket; + } } // last bucket, partially filled: - if let Ok(i) = usize::try_from(max_bucket.ceil() as i64) { + if let Ok(i) = usize::try_from(last_bucket as i64) { if let Some(bucket) = self.buckets.get_mut(i) { *bucket += last_bucket_factor * count_per_bucket; }