[repacker] add sanitization for GSUB/LookupList/Lookup during extension promotion.

src/graph/graph.hh

@@ -621,7 +621,6 @@ struct graph_t
     clone->obj.tail = tail;
     clone->distance = 0;
     clone->space = 0;
-    printf("Creating new node from %p to %p\n", head, tail);
 
     unsigned clone_idx = vertices_.length - 2;
 

src/graph/gsubgpos-graph.cc

@@ -28,6 +28,22 @@
 
 namespace graph {
 
+make_extension_context_t::make_extension_context_t (hb_tag_t table_tag_,
+                                                    graph_t& graph_,
+                                                    hb_vector_t<char>& buffer_)
+    : table_tag (table_tag_),
+      graph (graph_),
+      buffer (buffer_),
+      lookups ()
+{
+  GSTAR* gstar = graph::GSTAR::graph_to_gstar (graph_);
+  if (gstar)
+    gstar->find_lookups (graph, lookups);
+
+  unsigned extension_size = OT::ExtensionFormat1<OT::Layout::GSUB_impl::ExtensionSubst>::static_size;
+  buffer.alloc (num_non_ext_subtables () * extension_size);
+}
+
 unsigned make_extension_context_t::num_non_ext_subtables ()  {
   unsigned count = 0;
   for (auto l : lookups.values ())

src/graph/gsubgpos-graph.hh

@@ -35,63 +35,16 @@ namespace graph {
 
 struct Lookup;
 
-struct GSTAR : public OT::GSUBGPOS
-{
-  static GSTAR* graph_to_gstar (graph_t& graph)
-  {
-    return (GSTAR*) graph.root ().obj.head;
-  }
-
-  const void* get_lookup_list_field_offset () const
-  {
-    switch (u.version.major) {
-    case 1: return &(u.version1.lookupList);
-#ifndef HB_NO_BORING_EXPANSION
-    case 2: return &(u.version2.lookupList);
-#endif
-    default: return 0;
-    }
-  }
-
-  void find_lookups (graph_t& graph,
-                     hb_hashmap_t<unsigned, Lookup*>& lookups /* OUT */)
-  {
-    // TODO: template on types, based on gstar version.
-    unsigned lookup_list_idx = graph.index_for_offset (graph.root_idx (),
-                                                       get_lookup_list_field_offset());
-
-    const OT::LookupList<SmallTypes>* lookupList =
-        (const OT::LookupList<SmallTypes>*) graph.object (lookup_list_idx).head;
-
-    for (unsigned i = 0; i < lookupList->len; i++)
-    {
-      unsigned lookup_idx = graph.index_for_offset (lookup_list_idx, &(lookupList->arrayZ[i]));
-      Lookup* lookup = (Lookup*) graph.object (lookup_idx).head;
-      lookups.set (lookup_idx, lookup);
-    }
-  }
-};
-
 struct make_extension_context_t
 {
   hb_tag_t table_tag;
   graph_t& graph;
   hb_vector_t<char> buffer;
-  GSTAR* gstar;
   hb_hashmap_t<unsigned, graph::Lookup*> lookups;
 
-  make_extension_context_t (hb_tag_t table_tag_,
+  HB_INTERNAL make_extension_context_t (hb_tag_t table_tag_,
-                            graph_t& graph_)
+                                        graph_t& graph_,
-      : table_tag (table_tag_),
+                                        hb_vector_t<char>& buffer_);
-        graph (graph_),
-        buffer (),
-        gstar (graph::GSTAR::graph_to_gstar (graph_)),
-        lookups ()
-  {
-    gstar->find_lookups (graph, lookups);
-    unsigned extension_size = OT::ExtensionFormat1<OT::Layout::GSUB_impl::ExtensionSubst>::static_size;
-    buffer.alloc (num_non_ext_subtables () * extension_size);
-  }
 
   bool in_error () const
   {
@@ -99,7 +52,7 @@ struct make_extension_context_t
   }
 
  private:
-  unsigned num_non_ext_subtables ();
+  HB_INTERNAL unsigned num_non_ext_subtables ();
 };
 
 struct Lookup : public OT::Lookup
@@ -109,6 +62,13 @@ struct Lookup : public OT::Lookup
     return subTable.len;
   }
 
+  bool sanitize (graph_t::vertex_t& vertex) const
+  {
+    int64_t vertex_len = vertex.obj.tail - vertex.obj.head;
+    if (vertex_len < OT::Lookup::min_size) return false;
+    return vertex_len >= this->get_size ();
+  }
+
   bool is_extension (hb_tag_t table_tag) const
   {
     return lookupType == extension_type (table_tag);
@@ -123,14 +83,13 @@ struct Lookup : public OT::Lookup
     if (!ext_type || is_extension (c.table_tag))
     {
       // NOOP
-      printf("Already extension (obj %u).\n", this_index);
       return true;
     }
 
-    printf("Promoting lookup type %u (obj %u) to extension.\n",
+    DEBUG_MSG (SUBSET_REPACK, nullptr,
-           type,
+               "Promoting lookup type %u (obj %u) to extension.\n",
-           this_index);
+               type,
-
+               this_index);
 
     for (unsigned i = 0; i < subTable.len; i++)
     {
@@ -149,8 +108,6 @@ struct Lookup : public OT::Lookup
                                 unsigned lookup_index,
                                 unsigned subtable_index)
   {
-    printf("  Promoting subtable %u in lookup %u to extension.\n", subtable_index, lookup_index);
-
     unsigned type = lookupType;
     unsigned extension_size = OT::ExtensionFormat1<OT::Layout::GSUB_impl::ExtensionSubst>::static_size;
     unsigned start = c.buffer.length;
@@ -165,9 +122,6 @@ struct Lookup : public OT::Lookup
     extension->extensionLookupType = type;
     extension->extensionOffset = 0;
 
-    unsigned type_prime = extension->extensionLookupType;
-    printf("Assigned type %d to extension\n", type_prime);
-
     unsigned ext_index = c.graph.new_node (&c.buffer.arrayZ[start],
                                            &c.buffer.arrayZ[end]);
     if (ext_index == (unsigned) -1) return false;
@@ -176,11 +130,8 @@ struct Lookup : public OT::Lookup
     for (auto& l : lookup_vertex.obj.real_links.writer ())
     {
       if (l.objidx == subtable_index)
-      {
         // Change lookup to point at the extension.
-        printf("  Changing %d to %d\n", l.objidx, ext_index);
         l.objidx = ext_index;
-      }
     }
 
     // Make extension point at the subtable.
@@ -213,6 +164,73 @@ struct Lookup : public OT::Lookup
   }
 };
 
+template <typename T>
+struct LookupList : public OT::LookupList<T>
+{
+  bool sanitize (const graph_t::vertex_t& vertex) const
+  {
+    int64_t vertex_len = vertex.obj.tail - vertex.obj.head;
+    if (vertex_len < OT::LookupList<T>::min_size) return false;
+    return vertex_len >= OT::LookupList<T>::item_size * this->len;
+  }
+};
+
+struct GSTAR : public OT::GSUBGPOS
+{
+  static GSTAR* graph_to_gstar (graph_t& graph)
+  {
+    const auto& r = graph.root ();
+
+    GSTAR* gstar = (GSTAR*) r.obj.head;
+    if (!gstar->sanitize (r))
+      return nullptr;
+
+    return gstar;
+  }
+
+  const void* get_lookup_list_field_offset () const
+  {
+    switch (u.version.major) {
+    case 1: return &(u.version1.lookupList);
+#ifndef HB_NO_BORING_EXPANSION
+    case 2: return &(u.version2.lookupList);
+#endif
+    default: return 0;
+    }
+  }
+
+  bool sanitize (const graph_t::vertex_t& vertex)
+  {
+    int64_t len = vertex.obj.tail - vertex.obj.head;
+    // Only need access to fields in min_size
+    return len >= OT::GSUBGPOS::min_size;
+  }
+
+  void find_lookups (graph_t& graph,
+                     hb_hashmap_t<unsigned, Lookup*>& lookups /* OUT */)
+  {
+    // TODO: template on types, based on gstar version.
+    unsigned lookup_list_idx = graph.index_for_offset (graph.root_idx (),
+                                                       get_lookup_list_field_offset());
+
+    const LookupList<SmallTypes>* lookupList =
+        (const LookupList<SmallTypes>*) graph.object (lookup_list_idx).head;
+    if (!lookupList->sanitize (graph.vertices_[lookup_list_idx]))
+      return;
+
+    for (unsigned i = 0; i < lookupList->len; i++)
+    {
+      unsigned lookup_idx = graph.index_for_offset (lookup_list_idx, &(lookupList->arrayZ[i]));
+      Lookup* lookup = (Lookup*) graph.object (lookup_idx).head;
+      if (!lookup->sanitize (graph.vertices_[lookup_idx])) continue;
+      lookups.set (lookup_idx, lookup);
+    }
+  }
+};
+
+
+
+
 }
 
 #endif  /* GRAPH_GSUBGPOS_GRAPH_HH */

src/graph/serialize.hh

@@ -205,7 +205,6 @@ inline hb_blob_t* serialize (const graph_t& graph)
 {
   hb_vector_t<char> buffer;
   size_t size = graph.total_size_in_bytes ();
-  printf("Expected graph size: %lu.\n", size);
   if (!buffer.alloc (size)) {
     DEBUG_MSG (SUBSET_REPACK, nullptr, "Unable to allocate output buffer.");
     return nullptr;

src/hb-repacker.hh

@@ -41,8 +41,12 @@ using graph::graph_t;
  * docs/repacker.md
  */
 
+/*
+ * Analyze the lookups in a GSUB/GPOS table and decide if any should be promoted
+ * to extension lookups.
+ */
 static inline
-bool _make_extensions (graph::make_extension_context_t& ext_context)
+bool _promote_extensions_if_needed (graph::make_extension_context_t& ext_context)
 {
   // TODO: Move this into graph or gsubgpos graph?
   for (auto p : ext_context.lookups.iter ())
@@ -50,6 +54,7 @@ bool _make_extensions (graph::make_extension_context_t& ext_context)
     if (!p.second->make_extension (ext_context, p.first))
       return false;
   }
+
   return true;
 }
 
@@ -169,11 +174,8 @@ inline hb_blob_t*
 hb_resolve_overflows (const T& packed,
                       hb_tag_t table_tag,
                       unsigned max_rounds = 20) {
-  printf("Resolving overflows!\n");
   graph_t sorted_graph (packed);
   sorted_graph.sort_shortest_distance ();
-  printf("Initial graph size: %lu.\n",
-         sorted_graph.total_size_in_bytes ());
 
   bool will_overflow = graph::will_overflow (sorted_graph);
   if (!will_overflow)
@@ -181,16 +183,18 @@ hb_resolve_overflows (const T& packed,
     return graph::serialize (sorted_graph);
   }
 
-  graph::make_extension_context_t ext_context (table_tag, sorted_graph);
+  hb_vector_t<char> extension_buffer; // Needs to live until serialization is done.
-  if (ext_context.in_error ())
-    return nullptr;
 
   if ((table_tag == HB_OT_TAG_GPOS
        ||  table_tag == HB_OT_TAG_GSUB)
       && will_overflow)
   {
-    if (!_make_extensions (ext_context)) {
+    graph::make_extension_context_t ext_context (table_tag, sorted_graph, extension_buffer);
-      printf("make extensions failed.\n");
+    if (ext_context.in_error ())
+      return nullptr;
+
+    if (1 && !_promote_extensions_if_needed (ext_context)) {
+      DEBUG_MSG (SUBSET_REPACK, nullptr, "Extensions promotion failed.");
       return nullptr;
     }