From 9cac172f82dd492d068590434199989021be236e Mon Sep 17 00:00:00 2001 From: Behdad Esfahbod Date: Tue, 16 Mar 2010 03:46:17 -0400 Subject: [PATCH] [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_CHARP(&(X)) + (X).get_size ()))) #define NEXT(T,X) (*(reinterpret_cast(CHARP(&(X)) + (X).get_size ()))) -#define CONST_ARRAY_AFTER(T,X) ((reinterpret_cast(CONST_CHARP(&(X)) + sizeof (X)))) -#define ARRAY_AFTER(T,X) ((reinterpret_cast(CHARP(&(X)) + sizeof (X)))) +#define CONST_ARRAY_AFTER(T,X) ((reinterpret_cast(CONST_CHARP(&(X)) + X.get_size ()))) +#define ARRAY_AFTER(T,X) ((reinterpret_cast(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 struct Record { + static inline unsigned int get_size () { return sizeof (Record); } + 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*)values++).get_delta (x_ppem) << 6; + glyph_pos->x_pos += (base+*(OffsetTo*)values++).get_delta (x_ppem) << 16; else values++; } if (format & yPlaDevice) { if (y_ppem) - glyph_pos->y_pos += (base+*(OffsetTo*)values++).get_delta (y_ppem) << 6; + glyph_pos->y_pos += (base+*(OffsetTo*)values++).get_delta (y_ppem) << 16; else values++; } if (format & xAdvDevice) { if (x_ppem) - glyph_pos->x_advance += (base+*(OffsetTo*)values++).get_delta (x_ppem) << 6; + glyph_pos->x_advance += (base+*(OffsetTo*)values++).get_delta (x_ppem) << 16; else values++; } if (format & yAdvDevice) { if (y_ppem) - glyph_pos->y_advance += (base+*(OffsetTo*)values++).get_delta (y_ppem) << 6; + glyph_pos->y_advance += (base+*(OffsetTo*)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: -- 2.7.4