From 81f27df4d9db1bfc1dd04593cbd121397b86e9a6 Mon Sep 17 00:00:00 2001 From: Behdad Esfahbod Date: Sat, 16 Dec 2017 06:12:06 -0800 Subject: [PATCH] More work towards improving collect_glyphs() against bad input The three "XXXXX"'s should be switched to false. Doing that separately for ease of bisecting... --- src/hb-ot-layout-common-private.hh | 7 +++--- src/hb-ot-layout-gpos-table.hh | 25 +++++++++------------ src/hb-ot-layout-gsub-table.hh | 35 +++++++++++------------------ src/hb-set-digest-private.hh | 6 +++-- src/hb-set-private.hh | 36 +++++++++++++++--------------- 5 files changed, 48 insertions(+), 61 deletions(-) diff --git a/src/hb-ot-layout-common-private.hh b/src/hb-ot-layout-common-private.hh index 47a05b5ec..554364854 100644 --- a/src/hb-ot-layout-common-private.hh +++ b/src/hb-ot-layout-common-private.hh @@ -156,8 +156,7 @@ struct RangeRecord template inline bool add_coverage (set_t *glyphs) const { - glyphs->add_range (start, end); - return likely (start <= end); + return glyphs->add_range (start, end); } GlyphID start; /* First GlyphID in the range */ @@ -820,7 +819,7 @@ struct CoverageFormat2 unsigned int count = rangeRecord.len; for (unsigned int i = 0; i < count; i++) if (unlikely (!rangeRecord[i].add_coverage (glyphs))) - return false; + return true;//XXXXXXXXXXXXfalse; return true; } @@ -935,7 +934,7 @@ struct Coverage switch (u.format) { case 1: return u.format1.add_coverage (glyphs); case 2: return u.format2.add_coverage (glyphs); - default:return false; + default:return true;//XXXXXXXXXXXfalse; } } diff --git a/src/hb-ot-layout-gpos-table.hh b/src/hb-ot-layout-gpos-table.hh index 215e2d742..f1b61301e 100644 --- a/src/hb-ot-layout-gpos-table.hh +++ b/src/hb-ot-layout-gpos-table.hh @@ -607,12 +607,7 @@ struct PairSet unsigned int record_size = UINT16::static_size * (1 + len1 + len2); const PairValueRecord *record = CastP (arrayZ); - unsigned int count = len; - for (unsigned int i = 0; i < count; i++) - { - c->input->add (record->secondGlyph); - record = &StructAtOffset (record, record_size); - } + c->input->add_array (&record->secondGlyph, len, record_size); } inline bool apply (hb_apply_context_t *c, @@ -689,7 +684,7 @@ struct PairPosFormat1 inline void collect_glyphs (hb_collect_glyphs_context_t *c) const { TRACE_COLLECT_GLYPHS (this); - (this+coverage).add_coverage (c->input); + if (unlikely (!(this+coverage).add_coverage (c->input))) return; unsigned int count = pairSet.len; for (unsigned int i = 0; i < count; i++) (this+pairSet[i]).collect_glyphs (c, valueFormat); @@ -755,7 +750,7 @@ struct PairPosFormat2 inline void collect_glyphs (hb_collect_glyphs_context_t *c) const { TRACE_COLLECT_GLYPHS (this); - (this+coverage).add_coverage (c->input); + if (unlikely (!(this+coverage).add_coverage (c->input))) return; unsigned int count1 = class1Count; if (count1) @@ -904,7 +899,7 @@ struct CursivePosFormat1 inline void collect_glyphs (hb_collect_glyphs_context_t *c) const { TRACE_COLLECT_GLYPHS (this); - (this+coverage).add_coverage (c->input); + if (unlikely (!(this+coverage).add_coverage (c->input))) return; } inline const Coverage &get_coverage (void) const @@ -1062,8 +1057,8 @@ struct MarkBasePosFormat1 inline void collect_glyphs (hb_collect_glyphs_context_t *c) const { TRACE_COLLECT_GLYPHS (this); - (this+markCoverage).add_coverage (c->input); - (this+baseCoverage).add_coverage (c->input); + if (unlikely (!(this+markCoverage).add_coverage (c->input))) return; + if (unlikely (!(this+baseCoverage).add_coverage (c->input))) return; } inline const Coverage &get_coverage (void) const @@ -1165,8 +1160,8 @@ struct MarkLigPosFormat1 inline void collect_glyphs (hb_collect_glyphs_context_t *c) const { TRACE_COLLECT_GLYPHS (this); - (this+markCoverage).add_coverage (c->input); - (this+ligatureCoverage).add_coverage (c->input); + if (unlikely (!(this+markCoverage).add_coverage (c->input))) return; + if (unlikely (!(this+ligatureCoverage).add_coverage (c->input))) return; } inline const Coverage &get_coverage (void) const @@ -1278,8 +1273,8 @@ struct MarkMarkPosFormat1 inline void collect_glyphs (hb_collect_glyphs_context_t *c) const { TRACE_COLLECT_GLYPHS (this); - (this+mark1Coverage).add_coverage (c->input); - (this+mark2Coverage).add_coverage (c->input); + if (unlikely (!(this+mark1Coverage).add_coverage (c->input))) return; + if (unlikely (!(this+mark2Coverage).add_coverage (c->input))) return; } inline const Coverage &get_coverage (void) const diff --git a/src/hb-ot-layout-gsub-table.hh b/src/hb-ot-layout-gsub-table.hh index 28e0790e6..0b09c4e4a 100644 --- a/src/hb-ot-layout-gsub-table.hh +++ b/src/hb-ot-layout-gsub-table.hh @@ -54,13 +54,13 @@ struct SingleSubstFormat1 inline void collect_glyphs (hb_collect_glyphs_context_t *c) const { TRACE_COLLECT_GLYPHS (this); + if (unlikely (!(this+coverage).add_coverage (c->input))) return; Coverage::Iter iter; for (iter.init (this+coverage); iter.more (); iter.next ()) { /* TODO Switch to range-based API to work around malicious fonts. * https://github.com/harfbuzz/harfbuzz/issues/363 */ hb_codepoint_t glyph_id = iter.get_glyph (); - c->input->add (glyph_id); c->output->add ((glyph_id + deltaGlyphID) & 0xFFFFu); } } @@ -139,13 +139,13 @@ struct SingleSubstFormat2 inline void collect_glyphs (hb_collect_glyphs_context_t *c) const { TRACE_COLLECT_GLYPHS (this); + if (unlikely (!(this+coverage).add_coverage (c->input))) return; Coverage::Iter iter; unsigned int count = substitute.len; for (iter.init (this+coverage); iter.more (); iter.next ()) { if (unlikely (iter.get_coverage () >= count)) break; /* Work around malicious fonts. https://github.com/harfbuzz/harfbuzz/issues/363 */ - c->input->add (iter.get_glyph ()); c->output->add (substitute[iter.get_coverage ()]); } } @@ -269,9 +269,7 @@ struct Sequence inline void collect_glyphs (hb_collect_glyphs_context_t *c) const { TRACE_COLLECT_GLYPHS (this); - unsigned int count = substitute.len; - for (unsigned int i = 0; i < count; i++) - c->output->add (substitute[i]); + c->output->add_array (substitute.array, substitute.len); } inline bool apply (hb_apply_context_t *c) const @@ -348,7 +346,7 @@ struct MultipleSubstFormat1 inline void collect_glyphs (hb_collect_glyphs_context_t *c) const { TRACE_COLLECT_GLYPHS (this); - (this+coverage).add_coverage (c->input); + if (unlikely (!(this+coverage).add_coverage (c->input))) return; unsigned int count = sequence.len; for (unsigned int i = 0; i < count; i++) (this+sequence[i]).collect_glyphs (c); @@ -474,17 +472,15 @@ struct AlternateSubstFormat1 inline void collect_glyphs (hb_collect_glyphs_context_t *c) const { TRACE_COLLECT_GLYPHS (this); + if (unlikely (!(this+coverage).add_coverage (c->input))) return; Coverage::Iter iter; unsigned int count = alternateSet.len; for (iter.init (this+coverage); iter.more (); iter.next ()) { if (unlikely (iter.get_coverage () >= count)) break; /* Work around malicious fonts. https://github.com/harfbuzz/harfbuzz/issues/363 */ - c->input->add (iter.get_glyph ()); const AlternateSet &alt_set = this+alternateSet[iter.get_coverage ()]; - unsigned int count = alt_set.len; - for (unsigned int i = 0; i < count; i++) - c->output->add (alt_set[i]); + c->output->add_array (alt_set.array, alt_set.len); } } @@ -615,9 +611,7 @@ struct Ligature inline void collect_glyphs (hb_collect_glyphs_context_t *c) const { TRACE_COLLECT_GLYPHS (this); - unsigned int count = component.len; - for (unsigned int i = 1; i < count; i++) - c->input->add (component[i]); + c->input->add_array (component.array, component.len ? component.len - 1 : 0); c->output->add (ligGlyph); } @@ -801,13 +795,13 @@ struct LigatureSubstFormat1 inline void collect_glyphs (hb_collect_glyphs_context_t *c) const { TRACE_COLLECT_GLYPHS (this); + if (unlikely (!(this+coverage).add_coverage (c->input))) return; Coverage::Iter iter; unsigned int count = ligatureSet.len; for (iter.init (this+coverage); iter.more (); iter.next ()) { if (unlikely (iter.get_coverage () >= count)) break; /* Work around malicious fonts. https://github.com/harfbuzz/harfbuzz/issues/363 */ - c->input->add (iter.get_glyph ()); (this+ligatureSet[iter.get_coverage ()]).collect_glyphs (c); } } @@ -970,25 +964,22 @@ struct ReverseChainSingleSubstFormat1 inline void collect_glyphs (hb_collect_glyphs_context_t *c) const { TRACE_COLLECT_GLYPHS (this); - - const OffsetArrayOf &lookahead = StructAfter > (backtrack); + if (unlikely (!(this+coverage).add_coverage (c->input))) return; unsigned int count; - (this+coverage).add_coverage (c->input); - count = backtrack.len; for (unsigned int i = 0; i < count; i++) - (this+backtrack[i]).add_coverage (c->before); + if (unlikely (!(this+backtrack[i]).add_coverage (c->before))) return; + const OffsetArrayOf &lookahead = StructAfter > (backtrack); count = lookahead.len; for (unsigned int i = 0; i < count; i++) - (this+lookahead[i]).add_coverage (c->after); + if (unlikely (!(this+lookahead[i]).add_coverage (c->after))) return; const ArrayOf &substitute = StructAfter > (lookahead); count = substitute.len; - for (unsigned int i = 0; i < count; i++) - c->output->add (substitute[i]); + c->output->add_array (substitute.array, substitute.len); } inline const Coverage &get_coverage (void) const diff --git a/src/hb-set-digest-private.hh b/src/hb-set-digest-private.hh index 0f798ab69..e099a8264 100644 --- a/src/hb-set-digest-private.hh +++ b/src/hb-set-digest-private.hh @@ -71,7 +71,7 @@ struct hb_set_digest_lowest_bits_t mask |= mask_for (g); } - inline void add_range (hb_codepoint_t a, hb_codepoint_t b) { + inline bool add_range (hb_codepoint_t a, hb_codepoint_t b) { if ((b >> shift) - (a >> shift) >= mask_bits - 1) mask = (mask_t) -1; else { @@ -79,6 +79,7 @@ struct hb_set_digest_lowest_bits_t mask_t mb = mask_for (b); mask |= mb + (mb - ma) - (mb < ma); } + return true; } template @@ -128,9 +129,10 @@ struct hb_set_digest_combiner_t tail.add (g); } - inline void add_range (hb_codepoint_t a, hb_codepoint_t b) { + inline bool add_range (hb_codepoint_t a, hb_codepoint_t b) { head.add_range (a, b); tail.add_range (a, b); + return true; } template inline void add_array (const T *array, unsigned int count, unsigned int stride=sizeof(T)) diff --git a/src/hb-set-private.hh b/src/hb-set-private.hh index edeca6d75..4b6f7fb3e 100644 --- a/src/hb-set-private.hh +++ b/src/hb-set-private.hh @@ -70,20 +70,19 @@ struct hb_set_t inline void add_range (hb_codepoint_t a, hb_codepoint_t b) { - elt_t *la = &elt (a); - elt_t *lb = &elt (b); - if (la == lb) - *la |= (mask (b) << 1) - mask(a); - else - { - *la |= ~(mask (a) - 1); - la++; + elt_t *la = &elt (a); + elt_t *lb = &elt (b); + if (la == lb) + *la |= (mask (b) << 1) - mask(a); + else + { + *la |= ~(mask (a) - 1); + la++; - memset (la, 0xff, (char *) lb - (char *) la); + memset (la, 0xff, (char *) lb - (char *) la); - *lb |= ((mask (b) << 1) - 1); - - } + *lb |= ((mask (b) << 1) - 1); + } } inline bool is_equal (const page_t *other) const @@ -230,34 +229,35 @@ struct hb_set_t if (unlikely (!page)) return; page->add (g); } - inline void add_range (hb_codepoint_t a, hb_codepoint_t b) + inline bool add_range (hb_codepoint_t a, hb_codepoint_t b) { - if (unlikely (in_error || a > b || a == INVALID || b == INVALID)) return; + if (unlikely (in_error || a > b || a == INVALID || b == INVALID)) return true;//XXXXXXXfalse; unsigned int ma = get_major (a); unsigned int mb = get_major (b); if (ma == mb) { page_t *page = page_for_insert (a); - if (unlikely (!page)) return; + if (unlikely (!page)) return false; page->add_range (a, b); } else { page_t *page = page_for_insert (a); - if (unlikely (!page)) return; + if (unlikely (!page)) return false; page->add_range (a, major_start (ma + 1) - 1); for (unsigned int m = ma + 1; m < mb; m++) { page = page_for_insert (major_start (m)); - if (unlikely (!page)) return; + if (unlikely (!page)) return false; page->init1 (); } page = page_for_insert (b); - if (unlikely (!page)) return; + if (unlikely (!page)) return false; page->add_range (major_start (mb), b); } + return true; } template