From efda2f14e104eedfee7da50ba2d22ba9a9ae376b Mon Sep 17 00:00:00 2001
From: Garret Rieger <grieger@google.com>
Date: Fri, 24 Sep 2021 16:28:34 -0700
Subject: [PATCH] [repacker] fix bug in subgraph isolation.

Prior to this fix id remapping at the end of the isolation operation was fed the old subgraph instead of the new one. Which results in object indices being remapped for the nodes outside of the new subgraph. Adds a test which detects this problem.
---
 src/hb-repacker.hh   | 11 ++++++--
 src/test-repacker.cc | 64 +++++++++++++++++++++++++++++++++++++++++---
 2 files changed, 69 insertions(+), 6 deletions(-)

diff --git a/src/hb-repacker.hh b/src/hb-repacker.hh
index aac5cbdd8..fa2dd42b4 100644
--- a/src/hb-repacker.hh
+++ b/src/hb-repacker.hh
@@ -372,7 +372,7 @@ struct graph_t
     // incoming edges to root_idx should be all 32 bit in length so we don't need to de-dup these
     // set the subgraph incoming edge count to match all of root_idx's incoming edges
     subgraph.set (root_idx, vertices_[root_idx].incoming_edges);
-    find_subgraph(root_idx, subgraph);
+    find_subgraph (root_idx, subgraph);
 
     hb_hashmap_t<unsigned, unsigned> index_map;
     bool made_changes = false;
@@ -392,7 +392,14 @@ struct graph_t
     if (!made_changes)
       return false;
 
-    remap_obj_indices (index_map, subgraph.keys ());
+    auto new_subgraph =
+        + subgraph.keys ()
+        | hb_map([&] (unsigned node_idx) {
+          if (index_map.has (node_idx)) return index_map[node_idx];
+          return node_idx;
+        })
+        ;
+    remap_obj_indices (index_map, new_subgraph);
     return true;
   }
 
diff --git a/src/test-repacker.cc b/src/test-repacker.cc
index 6eb226344..32228f7a7 100644
--- a/src/test-repacker.cc
+++ b/src/test-repacker.cc
@@ -155,7 +155,7 @@ populate_serializer_with_isolation_overflow_complex (hb_serialize_context_t* c)
   add_offset (obj_f, c);
   unsigned obj_e = c->pop_pack ();
 
-  start_object ("c", 1, c);
+  start_object ("cc", 2, c);
   add_offset (obj_e, c);
   unsigned obj_c = c->pop_pack ();
 
@@ -180,6 +180,54 @@ populate_serializer_with_isolation_overflow_complex (hb_serialize_context_t* c)
   c->end_serialize();
 }
 
+static void
+populate_serializer_with_isolation_overflow_complex_expected (hb_serialize_context_t* c)
+{
+  std::string large_string(70000, 'a');
+  c->start_serialize<char> ();
+
+  // 32 bit subgraph
+  unsigned obj_f_prime = add_object ("f", 1, c);
+
+  start_object ("e", 1, c);
+  add_offset (obj_f_prime, c);
+  unsigned obj_e_prime = c->pop_pack ();
+
+  start_object ("cc", 2, c);
+  add_offset (obj_e_prime, c);
+  unsigned obj_c = c->pop_pack ();
+
+  start_object ("d", 1, c);
+  add_offset (obj_e_prime, c);
+  unsigned obj_d_prime = c->pop_pack ();
+
+  start_object (large_string.c_str(), 60000, c);
+  add_offset (obj_c, c);
+  add_offset (obj_d_prime, c);
+  unsigned obj_b = c->pop_pack ();
+
+  // 16 bit subgraph
+  unsigned obj_f = add_object ("f", 1, c);
+
+  start_object ("e", 1, c);
+  add_offset (obj_f, c);
+  unsigned obj_e = c->pop_pack ();
+
+  start_object ("d", 1, c);
+  add_offset (obj_e, c);
+  unsigned obj_d = c->pop_pack ();
+
+  start_object (large_string.c_str(), 10000, c);
+  add_offset (obj_d, c);
+  unsigned obj_g = c->pop_pack ();
+
+  start_object ("a", 1, c);
+  add_wide_offset (obj_b, c);
+  add_offset (obj_g, c);
+  c->pop_pack ();
+
+  c->end_serialize();
+}
 
 static void
 populate_serializer_with_isolation_overflow_spaces (hb_serialize_context_t* c)
@@ -600,12 +648,20 @@ static void test_resolve_overflows_via_isolation_with_recursive_duplication ()
   assert (!out.offset_overflow ());
   hb_bytes_t result = out.copy_bytes ();
 
-  unsigned expected_length = 8 + 10000 + 60000; // objects
-  expected_length += 4 + 2 * 9; // links
-  assert (result.length == expected_length);
+  void* expected_buffer = malloc (buffer_size);
+  hb_serialize_context_t e (expected_buffer, buffer_size);
+  assert (!e.offset_overflow ());
+  populate_serializer_with_isolation_overflow_complex_expected (&e);
+  hb_bytes_t expected_result = e.copy_bytes ();
+
+  assert (result.length == expected_result.length);
+  for (unsigned i = 0; i < result.length; i++)
+    assert (result[i] == expected_result[i]);
 
   result.fini ();
+  expected_result.fini ();
   free (buffer);
+  free (expected_buffer);
   free (out_buffer);
 }