From cb2681557019cbde4e18b2e02e5502ff74738384 Mon Sep 17 00:00:00 2001
From: Benson Joeris <bjoeris@ad.corp.google.com>
Date: Tue, 19 Feb 2019 10:35:47 -0500
Subject: [PATCH] Fix bug in `Intervals<T>::update()`

When the interval being updated went all the way to the end of the last
interval (UINT64_MAX), `update` would call `mergeLeft` on the `end()` interval.
This would cause `mergeLeft` to dereference an invalid iterator.

Also added a quick return for updating empty intervals, and unit tests to verify
correct handling of empty intervals, which were run both with and without this
optimization.

Also removed some unnecessary asserts that likely called many times. These
asserts could only fail if there was a bug in the `Intervals<T>` implementation,
which should be caught by the unit tests.
---
 renderdoc/core/intervals.h         | 15 +++++----------
 renderdoc/core/intervals_tests.cpp | 29 +++++++++++++++++++++++++++++
 2 files changed, 34 insertions(+), 10 deletions(-)

diff --git a/renderdoc/core/intervals.h b/renderdoc/core/intervals.h
index f1cc17283..f60ba9947 100644
--- a/renderdoc/core/intervals.h
+++ b/renderdoc/core/intervals.h
@@ -80,7 +80,6 @@ public:
   // If `x == start`, then `split(x)` is a no-op.
   inline void split(uint64_t x)
   {
-    RDCASSERT(this->start() <= x && x < this->finish());
     if(this->start() < x)
       this->iter = this->owner->insert(std::pair<uint64_t, T>(x, this->value())).first;
   }
@@ -194,7 +193,6 @@ public:
   {
     // Find the first interval starting after `x`; return the preceding interval.
     auto it = StartPoints.upper_bound(x);
-    RDCASSERT(it != StartPoints.begin());
     it--;
     return Wrap(it);
   }
@@ -204,7 +202,6 @@ public:
   {
     // Find the first interval starting after `x`; return the preceding interval.
     auto it = StartPoints.upper_bound(x);
-    RDCASSERT(it != StartPoints.begin());
     it--;
     return Wrap(it);
   }
@@ -216,6 +213,9 @@ public:
   template <typename Compose>
   void update(uint64_t start, uint64_t finish, T val, Compose comp)
   {
+    if(finish <= start)
+      return;
+
     auto i = find(start);
 
     // Split the interval so that `i.start == start`
@@ -240,7 +240,8 @@ public:
 
     // `i` now points to the interval following the last interval whose value was
     // modified; merge `i` with that last modified interval, if the values match.
-    i->mergeLeft();
+    if(i != end())
+      i->mergeLeft();
   }
 
   // Update `this` by composing the value of each interval with the value of the
@@ -262,8 +263,6 @@ public:
     //  * i.start() < j.end()
     while(true)
     {
-      RDCASSERT(i->start() >= j->start());
-      RDCASSERT(i->start() < j->finish());
       if(i->finish() > j->finish())
       {
         i->split(j->finish());
@@ -282,11 +281,7 @@ public:
       // in `other`, if necessary to maintain the invariant `i.start < j.end`.
       i++;
       if(i == end())
-      {
-        j++;
-        RDCASSERT(j == other.end());
         return;
-      }
       if(i->start() >= j->finish())
         j++;
     }
diff --git a/renderdoc/core/intervals_tests.cpp b/renderdoc/core/intervals_tests.cpp
index a12bc8960..50bd141c6 100644
--- a/renderdoc/core/intervals_tests.cpp
+++ b/renderdoc/core/intervals_tests.cpp
@@ -265,6 +265,35 @@ TEST_CASE("Test Intervals type", "[intervals]")
       test.update(0, UINT64_MAX, 1, [](uint64_t x, uint64_t y) -> uint64_t { return x + y; });
       check_intervals(test, {{0, 1, 5}, {5, 2, 10}, {10, 1, UINT64_MAX}});
     };
+
+    SECTION("update an empty interval in the interior of an interval")
+    {
+      Intervals<uint64_t> test = make_intervals({{0, 0, 5}, {5, 1, 10}, {10, 0, UINT64_MAX}});
+      test.update(2, 2, 1, [](uint64_t x, uint64_t y) -> uint64_t { return x + y; });
+      check_intervals(test, {{0, 0, 5}, {5, 1, 10}, {10, 0, UINT64_MAX}});
+    };
+
+    SECTION("update an empty interval on a boundary")
+    {
+      Intervals<uint64_t> test = make_intervals({{0, 0, 5}, {5, 1, 10}, {10, 0, UINT64_MAX}});
+      test.update(5, 5, 1, [](uint64_t x, uint64_t y) -> uint64_t { return x + y; });
+      check_intervals(test, {{0, 0, 5}, {5, 1, 10}, {10, 0, UINT64_MAX}});
+    };
+
+    SECTION("update an empty interval at 0")
+    {
+      Intervals<uint64_t> test = make_intervals({{0, 0, 5}, {5, 1, 10}, {10, 0, UINT64_MAX}});
+      test.update(0, 0, 1, [](uint64_t x, uint64_t y) -> uint64_t { return x + y; });
+      check_intervals(test, {{0, 0, 5}, {5, 1, 10}, {10, 0, UINT64_MAX}});
+    };
+
+    SECTION("update an empty interval at UINT64_MAX")
+    {
+      Intervals<uint64_t> test = make_intervals({{0, 0, 5}, {5, 1, 10}, {10, 0, UINT64_MAX}});
+      test.update(UINT64_MAX, UINT64_MAX, 1,
+                  [](uint64_t x, uint64_t y) -> uint64_t { return x + y; });
+      check_intervals(test, {{0, 0, 5}, {5, 1, 10}, {10, 0, UINT64_MAX}});
+    };
   };
 
   SECTION("mergeIntervals tests")