From 554ed06fac759508ad959482f784bf02e4839a66 Mon Sep 17 00:00:00 2001
From: Garret Rieger <grieger@google.com>
Date: Thu, 1 Dec 2022 21:51:17 +0000
Subject: [PATCH] [repacker] add cycle detection to the graph sort.

This allows us to bail early if the graph is not acyclic.
---
 src/graph/graph.hh                 | 14 ++++++++++----
 src/hb-repacker.hh                 |  5 +++++
 src/test-repacker.cc               |  1 +
 test/fuzzing/hb-repacker-fuzzer.cc | 17 +++++++++++++++--
 4 files changed, 31 insertions(+), 6 deletions(-)

diff --git a/src/graph/graph.hh b/src/graph/graph.hh
index 825a35594..66c07e54b 100644
--- a/src/graph/graph.hh
+++ b/src/graph/graph.hh
@@ -50,7 +50,7 @@ struct graph_t
     unsigned priority = 0;
 
 
-    bool link_positions_valid (unsigned parent_index)
+    bool link_positions_valid ()
     {
       hb_set_t assigned_bytes;
       for (const auto& l : obj.real_links)
@@ -321,8 +321,6 @@ struct graph_t
     vertices_scratch_.alloc (objects.length);
     for (unsigned i = 0; i < objects.length; i++)
     {
-      // TODO(grieger): check all links point to valid objects.
-
       // If this graph came from a serialization buffer object 0 is the
       // nil object. We don't need it for our purposes here so drop it.
       if (i == 0 && !objects[i])
@@ -335,7 +333,8 @@ struct graph_t
       if (check_success (!vertices_.in_error ()))
         v->obj = *objects[i];
 
-      check_success (v->link_positions_valid (i));
+      check_success (v->link_positions_valid ());
+
       if (!removed_nil) continue;
       // Fix indices to account for removed nil object.
       for (auto& l : v->obj.all_links_writer ()) {
@@ -455,6 +454,13 @@ struct graph_t
       hb_swap (sorted_graph[new_id], vertices_[next_id]);
       const vertex_t& next = sorted_graph[new_id];
 
+      if (unlikely (!check_success(new_id >= 0))) {
+        // We are out of ids. Which means we've visited a node more than once.
+        // This graph contains a cycle which is not allowed.
+        DEBUG_MSG (SUBSET_REPACK, nullptr, "Invalid graph. Contains cycle.");
+        return;
+      }
+
       id_map[next_id] = new_id--;
 
       for (const auto& link : next.obj.all_links ()) {
diff --git a/src/hb-repacker.hh b/src/hb-repacker.hh
index 1abb95760..6817ffae4 100644
--- a/src/hb-repacker.hh
+++ b/src/hb-repacker.hh
@@ -283,6 +283,11 @@ hb_resolve_graph_overflows (hb_tag_t table_tag,
                             graph_t& sorted_graph /* IN/OUT */)
 {
   sorted_graph.sort_shortest_distance ();
+  if (sorted_graph.in_error ())
+  {
+    DEBUG_MSG (SUBSET_REPACK, nullptr, "Sorted graph in error state after initial sort.");
+    return false;
+  }
 
   bool will_overflow = graph::will_overflow (sorted_graph);
   if (!will_overflow)
diff --git a/src/test-repacker.cc b/src/test-repacker.cc
index 7a20ba52c..94ff0849a 100644
--- a/src/test-repacker.cc
+++ b/src/test-repacker.cc
@@ -1476,6 +1476,7 @@ static void test_sort_shortest ()
 
   graph_t graph (c.object_graph ());
   graph.sort_shortest_distance ();
+  assert (!graph.in_error ());
 
   assert(strncmp (graph.object (4).head, "abc", 3) == 0);
   assert(graph.object (4).real_links.length == 3);
diff --git a/test/fuzzing/hb-repacker-fuzzer.cc b/test/fuzzing/hb-repacker-fuzzer.cc
index b670af6e8..536b0e8d0 100644
--- a/test/fuzzing/hb-repacker-fuzzer.cc
+++ b/test/fuzzing/hb-repacker-fuzzer.cc
@@ -15,6 +15,20 @@ typedef struct
   uint8_t width;
 } link_t;
 
+/* The fuzzer seed contains a serialized representation of a object graph which forms
+ * the input graph to the repacker call. The binary format is:
+ *
+ * table tag: 4 bytes
+ * number of objects: 2 bytes
+ * objects[number of objects]:
+ *   blob size: 2 bytes
+ *   blob: blob size bytes
+ * num of real links: 2 bytes
+ * links[number of real links]: link_t struct
+ *
+ * TODO(garretrieger): add optional virtual links
+ */
+
 template <typename T>
 bool read(const uint8_t** data, size_t* size, T* out)
 {
@@ -108,8 +122,7 @@ extern "C" int LLVMFuzzerTestOneInput (const uint8_t *data, size_t size)
   {
     if (!read<link_t> (&data, &size, &links[i])) goto end;
 
-    if (links[i].parent >= num_objects
-        || links[i].child > links[i].parent) // Enforces DAG graph
+    if (links[i].parent >= num_objects)
       goto end;
   }