commit efda2f14e104eedfee7da50ba2d22ba9a9ae376b
author Garret Rieger <grieger@google.com> Fri Sep 24 16:28:34 2021 -0700
committer Garret Rieger <grieger@google.com> Fri Sep 24 17:53:08 2021 -0600
tree 2210f13f74fe28bee52f069496ac1553eff3fadf
parent 6f1909439862cc37246aba55a85c0a072b945dff

[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

diff --git a/src/hb-repacker.hh b/src/hb-repacker.hh
index aac5cbd..fa2dd42 100644
--- a/src/hb-repacker.hh
+++ b/src/hb-repacker.hh
@@ -372,7 +372,7 @@
     // 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 @@
     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;
   }
 

src/test-repacker.cc

diff --git a/src/test-repacker.cc b/src/test-repacker.cc
index 6eb2263..32228f7 100644
--- a/src/test-repacker.cc
+++ b/src/test-repacker.cc
@@ -155,7 +155,7 @@
   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 @@
   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 @@
   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);
 }