GNOME.org
 

[pango] [HB] Fix the mystery bug!

commit 9cac172f82dd492d068590434199989021be236e
Author: Behdad Esfahbod <behdad behdad org>
Date:   Tue Mar 16 03:46:17 2010 -0400

    [HB] Fix the mystery bug!
    
    A couple bugs joined forces to exhibit the mystery behavior of
    crashes / infinite loops on OS X / wrong kerning / invalid memory
    access.  Pooh!
    
    The bugs were involved:
    
      - Wrong pointer math with ValueRecord in PairPosFormat1
    
      - Fallout from avoiding flex arrays, code not correctly updated
        to remove sizeof() usage.
    
    We strictly never use sizeof() directly now.  And the PairPos code
    is cleaned up.  Should fix them all.  Bugs are:
    
      Bug 605655 - Pango 1.26.2 introduces kerning bug
      Bug 611229 - Pango reads from uninitialized memory
      Bug 593240 - (pangoosx) Crash / infinite loop with Mac OS X
    
    We were also doing wrong math converting Device adjustments to
    hb_position_t.  Fallout from FreeType days.  Should shift 16, not
    6.  Fixed that too.
    
    There's still another bug: we don't sanitize Device records
    referenced from value records.  Fixing that also.

 pango/opentype/hb-open-file-private.hh          |    4 ++-
 pango/opentype/hb-open-type-private.hh          |   11 ++++---
 pango/opentype/hb-ot-layout-common-private.hh   |   10 +++++-
 pango/opentype/hb-ot-layout-gpos-private.hh     |   37 +++++++++++++----------
 pango/opentype/hb-ot-layout-gsubgpos-private.hh |   15 +++++----
 5 files changed, 46 insertions(+), 31 deletions(-)
---
diff --git a/pango/opentype/hb-open-file-private.hh b/pango/opentype/hb-open-file-private.hh
index dda6b94..0947747 100644
--- a/pango/opentype/hb-open-file-private.hh
+++ b/pango/opentype/hb-open-file-private.hh
@@ -47,6 +47,8 @@ struct TTCHeader;
 
 typedef struct TableDirectory
 {
+  static inline unsigned int get_size () { return sizeof (TableDirectory); }
+
   inline bool sanitize (SANITIZE_ARG_DEF, const void *base) {
     TRACE_SANITIZE ();
     return SANITIZE_SELF () && SANITIZE (tag) &&
@@ -108,7 +110,7 @@ typedef struct OffsetTable
   public:
   inline bool sanitize (SANITIZE_ARG_DEF, const void *base) {
     TRACE_SANITIZE ();
-    if (!(SANITIZE_SELF () && SANITIZE_MEM (tableDir, sizeof (tableDir[0]) * numTables))) return false;
+    if (!(SANITIZE_SELF () && SANITIZE_MEM (tableDir, tableDir[0].get_size () * numTables))) return false;
     unsigned int count = numTables;
     for (unsigned int i = 0; i < count; i++)
       if (!SANITIZE_BASE (tableDir[i], base))
diff --git a/pango/opentype/hb-open-type-private.hh b/pango/opentype/hb-open-type-private.hh
index f460f1a..06324ee 100644
--- a/pango/opentype/hb-open-type-private.hh
+++ b/pango/opentype/hb-open-type-private.hh
@@ -50,8 +50,8 @@
 #define CONST_NEXT(T,X)		(*(reinterpret_cast<const T *>(CONST_CHARP(&(X)) + (X).get_size ())))
 #define NEXT(T,X)		(*(reinterpret_cast<T *>(CHARP(&(X)) + (X).get_size ())))
 
-#define CONST_ARRAY_AFTER(T,X)	((reinterpret_cast<const T *>(CONST_CHARP(&(X)) + sizeof (X))))
-#define ARRAY_AFTER(T,X)	((reinterpret_cast<T *>(CHARP(&(X)) + sizeof (X))))
+#define CONST_ARRAY_AFTER(T,X)	((reinterpret_cast<const T *>(CONST_CHARP(&(X)) + X.get_size ())))
+#define ARRAY_AFTER(T,X)	((reinterpret_cast<T *>(CHARP(&(X)) + X.get_size ())))
 
 /*
  * Class features
@@ -362,6 +362,7 @@ struct Sanitizer
 #define DEFINE_INT_TYPE1(NAME, TYPE, BIG_ENDIAN, BYTES) \
   struct NAME \
   { \
+    static inline unsigned int get_size () { return BYTES; } \
     inline NAME& set (TYPE i) { BIG_ENDIAN##_put_unaligned(v, i); return *this; } \
     inline operator TYPE(void) const { return BIG_ENDIAN##_get_unaligned (v); } \
     inline bool operator== (NAME o) const { return BIG_ENDIAN##_cmp_unaligned (v, o.v); } \
@@ -424,7 +425,7 @@ struct CheckSum : ULONG
   static uint32_t CalcTableChecksum (ULONG *Table, uint32_t Length)
   {
     uint32_t Sum = 0L;
-    ULONG *EndPtr = Table+((Length+3) & ~3) / sizeof(ULONG);
+    ULONG *EndPtr = Table+((Length+3) & ~3) / ULONG::get_size ();
 
     while (Table < EndPtr)
       Sum += *Table++;
@@ -517,7 +518,7 @@ struct GenericArrayOf
     return const_array()[i];
   }
   inline unsigned int get_size () const
-  { return sizeof (len) + len * sizeof (Type); }
+  { return len.get_size () + len * Type::get_size (); }
 
   inline bool sanitize (SANITIZE_ARG_DEF) {
     TRACE_SANITIZE ();
@@ -620,7 +621,7 @@ struct HeadlessArrayOf
     return const_array()[i-1];
   }
   inline unsigned int get_size () const
-  { return sizeof (len) + (len ? len - 1 : 0) * sizeof (Type); }
+  { return len.get_size () + (len ? len - 1 : 0) * Type::get_size (); }
 
   inline bool sanitize (SANITIZE_ARG_DEF) {
     TRACE_SANITIZE ();
diff --git a/pango/opentype/hb-ot-layout-common-private.hh b/pango/opentype/hb-ot-layout-common-private.hh
index 442aad2..aff1468 100644
--- a/pango/opentype/hb-ot-layout-common-private.hh
+++ b/pango/opentype/hb-ot-layout-common-private.hh
@@ -51,6 +51,8 @@
 template <typename Type>
 struct Record
 {
+  static inline unsigned int get_size () { return sizeof (Record<Type>); }
+
   inline bool sanitize (SANITIZE_ARG_DEF, const void *base) {
     TRACE_SANITIZE ();
     return SANITIZE (tag) && SANITIZE_BASE (offset, base);
@@ -348,6 +350,8 @@ struct CoverageRangeRecord
 {
   friend struct CoverageFormat2;
 
+  static inline unsigned int get_size () { return sizeof (CoverageRangeRecord); }
+
   private:
   inline unsigned int get_coverage (hb_codepoint_t glyph_id) const
   {
@@ -466,6 +470,8 @@ struct ClassRangeRecord
 {
   friend struct ClassDefFormat2;
 
+  static inline unsigned int get_size () { return sizeof (ClassRangeRecord); }
+
   private:
   inline hb_ot_layout_class_t get_class (hb_codepoint_t glyph_id) const
   {
@@ -583,8 +589,8 @@ struct Device
   inline unsigned int get_size () const
   {
     unsigned int f = deltaFormat;
-    if (HB_UNLIKELY (f < 1 || f > 3 || startSize > endSize)) return sizeof (*this);
-    return sizeof (*this) + ((endSize - startSize + (1 << (4 - f)) - 1) >> (4 - f));
+    if (HB_UNLIKELY (f < 1 || f > 3 || startSize > endSize)) return 3 * USHORT::get_size ();
+    return 3 * USHORT::get_size () + ((endSize - startSize + (1 << (4 - f)) - 1) >> (4 - f));
   }
 
   inline bool sanitize (SANITIZE_ARG_DEF) {
diff --git a/pango/opentype/hb-ot-layout-gpos-private.hh b/pango/opentype/hb-ot-layout-gpos-private.hh
index 81b7fa1..be7b5a5 100644
--- a/pango/opentype/hb-ot-layout-gpos-private.hh
+++ b/pango/opentype/hb-ot-layout-gpos-private.hh
@@ -57,7 +57,7 @@ struct ValueFormat : USHORT
   inline unsigned int get_len () const
   { return _hb_popcount32 ((unsigned int) *this); }
   inline unsigned int get_size () const
-  { return get_len () * sizeof (Value); }
+  { return get_len () * Value::get_size (); }
 
   void apply_value (hb_ot_layout_context_t *context,
 		    const char          *base,
@@ -117,25 +117,25 @@ struct ValueRecord {
     /* pixel -> fractional pixel */
     if (format & xPlaDevice) {
       if (x_ppem)
-	glyph_pos->x_pos += (base+*(OffsetTo<Device>*)values++).get_delta (x_ppem) << 6;
+	glyph_pos->x_pos += (base+*(OffsetTo<Device>*)values++).get_delta (x_ppem) << 16;
       else
         values++;
     }
     if (format & yPlaDevice) {
       if (y_ppem)
-	glyph_pos->y_pos += (base+*(OffsetTo<Device>*)values++).get_delta (y_ppem) << 6;
+	glyph_pos->y_pos += (base+*(OffsetTo<Device>*)values++).get_delta (y_ppem) << 16;
       else
         values++;
     }
     if (format & xAdvDevice) {
       if (x_ppem)
-	glyph_pos->x_advance += (base+*(OffsetTo<Device>*)values++).get_delta (x_ppem) << 6;
+	glyph_pos->x_advance += (base+*(OffsetTo<Device>*)values++).get_delta (x_ppem) << 16;
       else
         values++;
     }
     if (format & yAdvDevice) {
       if (y_ppem)
-	glyph_pos->y_advance += (base+*(OffsetTo<Device>*)values++).get_delta (y_ppem) << 6;
+	glyph_pos->y_advance += (base+*(OffsetTo<Device>*)values++).get_delta (y_ppem) << 16;
       else
         values++;
     }
@@ -205,10 +205,11 @@ struct AnchorFormat3
       *x = _hb_16dot16_mul_trunc (context->font->x_scale, xCoordinate);
       *y = _hb_16dot16_mul_trunc (context->font->y_scale, yCoordinate);
 
+      /* pixel -> fractional pixel */
       if (context->font->x_ppem)
-	*x += (this+xDeviceTable).get_delta (context->font->x_ppem) << 6;
+	*x += (this+xDeviceTable).get_delta (context->font->x_ppem) << 16;
       if (context->font->y_ppem)
-	*y += (this+yDeviceTable).get_delta (context->font->y_ppem) << 6;
+	*y += (this+yDeviceTable).get_delta (context->font->y_ppem) << 16;
   }
 
   inline bool sanitize (SANITIZE_ARG_DEF) {
@@ -277,7 +278,7 @@ struct AnchorMatrix
     TRACE_SANITIZE ();
     if (!SANITIZE_SELF ()) return false;
     unsigned int count = rows * cols;
-    if (!SANITIZE_ARRAY (matrix, sizeof (matrix[0]), count)) return false;
+    if (!SANITIZE_ARRAY (matrix, matrix[0].get_size (), count)) return false;
     for (unsigned int i = 0; i < count; i++)
       if (!SANITIZE_THIS (matrix[i])) return false;
     return true;
@@ -296,6 +297,8 @@ struct MarkRecord
 {
   friend struct MarkArray;
 
+  static inline unsigned int get_size () { return sizeof (MarkRecord); }
+
   inline bool sanitize (SANITIZE_ARG_DEF, const void *base) {
     TRACE_SANITIZE ();
     return SANITIZE_SELF () && SANITIZE_BASE (markAnchor, base);
@@ -406,7 +409,7 @@ struct SinglePosFormat2
       return false;
 
     valueFormat.apply_value (context, CONST_CHARP(this),
-			     values + index * valueFormat.get_len (),
+			     &values[index * valueFormat.get_len ()],
 			     CURPOSITION ());
 
     buffer->in_pos++;
@@ -487,7 +490,7 @@ struct PairSet
     TRACE_SANITIZE ();
     if (!SANITIZE_SELF ()) return false;
     unsigned int count = (1 + format_len) * len;
-    return SANITIZE_MEM (array, sizeof (array[0]) * count);
+    return SANITIZE_MEM (array, USHORT::get_size () * count);
   }
 
   private:
@@ -526,7 +529,7 @@ struct PairPosFormat1
 
     unsigned int len1 = valueFormat1.get_len ();
     unsigned int len2 = valueFormat2.get_len ();
-    unsigned int record_len = 1 + len1 + len2;
+    unsigned int record_size = USHORT::get_size () * (1 + len1 + len2);
 
     unsigned int count = pair_set.len;
     const PairValueRecord *record = pair_set.array;
@@ -534,14 +537,14 @@ struct PairPosFormat1
     {
       if (IN_GLYPH (j) == record->secondGlyph)
       {
-	valueFormat1.apply_value (context, CONST_CHARP(this), record->values, CURPOSITION ());
-	valueFormat2.apply_value (context, CONST_CHARP(this), record->values + len1, POSITION (j));
+	valueFormat1.apply_value (context, CONST_CHARP(this), &record->values[0], CURPOSITION ());
+	valueFormat2.apply_value (context, CONST_CHARP(this), &record->values[len1], POSITION (j));
 	if (len2)
 	  j++;
 	buffer->in_pos = j;
 	return true;
       }
-      record += record_len;
+      record = &CONST_CAST (PairValueRecord, *record, record_size);
     }
 
     return false;
@@ -604,7 +607,7 @@ struct PairPosFormat2
     if (HB_UNLIKELY (klass1 >= class1Count || klass2 >= class2Count))
       return false;
 
-    const Value *v = values + record_len * (klass1 * class2Count + klass2);
+    const Value *v = &values[record_len * (klass1 * class2Count + klass2)];
     valueFormat1.apply_value (context, CONST_CHARP(this), v, CURPOSITION ());
     valueFormat2.apply_value (context, CONST_CHARP(this), v + len1, POSITION (j));
 
@@ -620,7 +623,7 @@ struct PairPosFormat2
     if (!(SANITIZE_SELF () && SANITIZE_THIS (coverage) &&
 	  SANITIZE_THIS2 (classDef1, classDef2))) return false;
 
-    unsigned int record_size =valueFormat1.get_size () + valueFormat2.get_size ();
+    unsigned int record_size = valueFormat1.get_size () + valueFormat2.get_size ();
     unsigned int len = class1Count * class2Count;
     return SANITIZE_ARRAY (values, record_size, len);
   }
@@ -690,6 +693,8 @@ struct PairPos
 
 struct EntryExitRecord
 {
+  static inline unsigned int get_size () { return sizeof (EntryExitRecord); }
+
   inline bool sanitize (SANITIZE_ARG_DEF, const void *base) {
     TRACE_SANITIZE ();
     return SANITIZE_BASE2 (entryAnchor, exitAnchor, base);
diff --git a/pango/opentype/hb-ot-layout-gsubgpos-private.hh b/pango/opentype/hb-ot-layout-gsubgpos-private.hh
index 91703da..0e15cf6 100644
--- a/pango/opentype/hb-ot-layout-gsubgpos-private.hh
+++ b/pango/opentype/hb-ot-layout-gsubgpos-private.hh
@@ -194,7 +194,8 @@ static inline bool match_lookahead (APPLY_ARG_DEF,
 
 struct LookupRecord
 {
-  public:
+  static inline unsigned int get_size () { return sizeof (LookupRecord); }
+
   inline bool sanitize (SANITIZE_ARG_DEF) {
     TRACE_SANITIZE ();
     return SANITIZE_SELF ();
@@ -297,7 +298,7 @@ struct Rule
   inline bool apply (APPLY_ARG_DEF, ContextLookupContext &lookup_context) const
   {
     TRACE_APPLY ();
-    const LookupRecord *lookupRecord = &CONST_CAST (LookupRecord, input, sizeof (input[0]) * (inputCount ? inputCount - 1 : 0));
+    const LookupRecord *lookupRecord = &CONST_CAST (LookupRecord, input, input[0].get_size () * (inputCount ? inputCount - 1 : 0));
     return context_lookup (APPLY_ARG,
 			   inputCount, input,
 			   lookupCount, lookupRecord,
@@ -309,8 +310,8 @@ struct Rule
     TRACE_SANITIZE ();
     if (!(SANITIZE (inputCount) && SANITIZE (lookupCount))) return false;
     return SANITIZE_MEM (input,
-			 sizeof (input[0]) * inputCount +
-			 sizeof (lookupRecordX[0]) * lookupCount);
+			 input[0].get_size () * inputCount +
+			 lookupRecordX[0].get_size () * lookupCount);
   }
 
   private:
@@ -446,7 +447,7 @@ struct ContextFormat3
     if (HB_LIKELY (index == NOT_COVERED))
       return false;
 
-    const LookupRecord *lookupRecord = &CONST_CAST(LookupRecord, coverage, sizeof (coverage[0]) * glyphCount);
+    const LookupRecord *lookupRecord = &CONST_CAST(LookupRecord, coverage, coverage[0].get_size () * glyphCount);
     struct ContextLookupContext lookup_context = {
       {match_coverage, apply_func},
       DECONST_CHARP(this)
@@ -463,8 +464,8 @@ struct ContextFormat3
     unsigned int count = glyphCount;
     for (unsigned int i = 0; i < count; i++)
       if (!SANITIZE_THIS (coverage[i])) return false;
-    LookupRecord *lookupRecord = &CAST(LookupRecord, coverage, sizeof (coverage[0]) * glyphCount);
-    return SANITIZE_MEM (lookupRecord, sizeof (lookupRecord[0]) * lookupCount);
+    LookupRecord *lookupRecord = &CAST(LookupRecord, coverage, coverage[0].get_size () * glyphCount);
+    return SANITIZE_MEM (lookupRecord, lookupRecord[0].get_size () * lookupCount);
   }
 
   private:
[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]