walkero
/
harfbuzz
1
0
Fork 0
Code Issues Pull Requests Projects Releases Wiki Activity

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.
This commit is contained in:
Behdad Esfahbod 2010-03-16 03:46:17 -04:00
parent 9b39755d10
commit 4b8487d83e
5 changed files with 46 additions and 31 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

4
src/hb-open-file-private.hh
View File

@ -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))

11
src/hb-open-type-private.hh
View File

@ -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
@ -363,6 +363,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); } \
@ -425,7 +426,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++;
@ -530,7 +531,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 ();
@ -633,7 +634,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 ();

10
src/hb-ot-layout-common-private.hh
View File

@ -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);
@ -353,6 +355,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
{
@ -471,6 +475,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
{
@ -588,8 +594,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) {

37
src/hb-ot-layout-gpos-private.hh
View File

@ -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,
@ -112,16 +112,16 @@ struct ValueRecord {
y_ppem = context->font->y_ppem;
/* pixel -> fractional pixel */
if (format & xPlaDevice) {
if (x_ppem) glyph_pos->x_offset += (base+*(OffsetTo<Device>*)values++).get_delta (x_ppem) << 6; else values++;
if (x_ppem) glyph_pos->x_offset += (base+*(OffsetTo<Device>*)values++).get_delta (x_ppem) << 16; else values++;
}
if (format & yPlaDevice) {
if (y_ppem) glyph_pos->y_offset += (base+*(OffsetTo<Device>*)values++).get_delta (y_ppem) << 6; else values++;
if (y_ppem) glyph_pos->y_offset += (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; else values++;
if (x_ppem) 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; else values++;
if (y_ppem) glyph_pos->y_advance += (base+*(OffsetTo<Device>*)values++).get_delta (y_ppem) << 16; else values++;
}
}
};
@ -190,10 +190,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) {
@ -262,7 +263,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;
@ -281,6 +282,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);
@ -391,7 +394,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++;
@ -472,7 +475,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:
@ -511,7 +514,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;
@ -519,14 +522,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;
@ -589,7 +592,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));
@ -605,7 +608,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);
}
@ -675,6 +678,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);

15
src/hb-ot-layout-gsubgpos-private.hh
View File

@ -195,7 +195,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 ();
@ -298,7 +299,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,
@ -310,8 +311,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:
@ -447,7 +448,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)
@ -464,8 +465,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: