Closed Bug 580719 Opened 14 years ago Closed 14 years ago

Arithmetic exception leads to crash [@hb_ot_layout_context_t::scale_x]

Categories

(Core :: Graphics, defect)

x86_64
macOS
defect
Not set
critical

Tracking

()

RESOLVED FIXED
Tracking Status
blocking2.0 --- final+

People

(Reporter: posidron, Assigned: jfkthame)

References

(Blocks 1 open bug)

Details

Attachments

(7 files, 1 obsolete file)

Attached file testcase
Build identifier: Mozilla/5.0 (Macintosh; Intel Mac OS X 10.6; en-US;
rv:2.0b2pre) Gecko/20100718 Minefield/4.0b2pre

Program received signal EXC_ARITHMETIC, Arithmetic exception.
0x00000001017cfd42 in hb_ot_layout_context_t::scale_x (this=0x7fff5fbf3880, v=0) at hb-ot-layout-private.hh:80
80	  inline hb_position_t scale_x (int16_t v) { return (int64_t) this->font->x_scale * v / this->face->head_table->unitsPerEm; }

Load the provided html file.
Attached file callstack (obsolete) —
blocking2.0: --- → ?
Summary: Crash because of division by zero [@ hb_ot_layout_context_t::scale_x] → Arithmetic exception leads to crash [@hb_ot_layout_context_t::scale_x]
blocking2.0: ? → final+
A lot of the profile is in drawing. I wonder if the dirty regions while typing are wrong?
Assignee: nobody → roc
Crap, wrong bug, sorry.
Assignee: roc → jfkthame
I am not able to reproduce this since other font robustness fixes have landed recently. Please reconfirm with current trunk build; we may be able to resolve this one as already fixed.
Confirmed.
(In reply to comment #5)
> Confirmed.

Sorry to re-ask, but could you make that clearer, please - did you mean you're confirming that you still see the crash, or that it's now fixed?
Hi Jonathan,
I wasn't able to reproduce it, seems fixed.
OK, thanks. Resolving this as Fixed.

I believe this particular testcase was initially fixed as a result of the patch in bug 580212, which causes the "bad" font to be rejected because its 'loca' table is invalid. The patch in bug 580863 would also have fixed this because it detects and rejects the bad upem value in the 'head' table, which was the specific cause of the actual crash here.
Status: NEW → RESOLVED
Closed: 14 years ago
Resolution: --- → FIXED
Attached file testcase-new
Reopened because this new testcase crashes at the exact same location.
Assignee: jfkthame → christoph.diehl
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Attached file callstack
Attachment #459077 - Attachment is obsolete: true
Number of replaced values: 4
Offset:    284/0x00011c	Value: ['22', '9f', 'a8'] (head)
Offset:    775/0x000307	Value: ['22', '9f', 'a8'] (hmtx)
Offset:  34378/0x00864a	Value: ['22', '9f', 'a8'] (glyf)
Offset:  74589/0x01235d	Value: ['75', '13', '6f'] (glyf)

Tag: b'head' Checksum: 0xf2f37c3f Offset:    284/0x0000011c Length: 54
Tag: b'hmtx' Checksum: 0xa6f73c27 Offset:    496/0x000001f0 Length: 2492
Tag: b'glyf' Checksum: 0xa6c28b35 Offset:   5808/0x000016b0 Length: 101752
This prevents harfbuzz trying to use a null blob (due to sanitize() rejection) as a head table (see message on the harfbuzz mailing list).
Assignee: christoph.diehl → jfkthame
Attachment #466296 - Flags: review?(jdaggett)
Fixing the head table validation exposed a potential null-deref in hb-ot-layout.cc (see message to harfbuzz mailing list).
Attachment #466297 - Flags: review?(jdaggett)
We could have avoided the harfbuzz issue here with slightly stricter validation of the 'head' table before loading a downloaded font. I think we should add a sanity-check of the tableVersionNumber field, in addition to the harfbuzz fixes (even though they will prevent us actually crashing on this testcase).
Attachment #466298 - Flags: review?(jdaggett)
Attachment #466297 - Flags: review?(jdaggett) → review+
Attachment #466298 - Flags: review?(jdaggett) → review+
Comment on attachment 466296 [details] [diff] [review]
patch, part 1 - [harfbuzz] check head table validity when creating face

Looks good.  It would be good to have a testcase that covers these cases.
Attachment #466296 - Flags: review?(jdaggett) → review+
http://hg.mozilla.org/mozilla-central/rev/4fd842de2365 (pt1)
http://hg.mozilla.org/mozilla-central/rev/67c26c853bb7 (pt2)
http://hg.mozilla.org/mozilla-central/rev/007d994cac53 (pt3)

I plan to push a crashtest based on attachment 466256 [details] as a follow-up.
Status: REOPENED → RESOLVED
Closed: 14 years ago14 years ago
Resolution: --- → FIXED
Attached patch crashtestSplinter Review
This is simply attachment 466256 [details] in crashtest form, to verify that it no longer crashes. (It does not actually test ALL the individual fixes here, because the sanity-check in part 3 now prevents the font getting as far as the other points of failure within harfbuzz.)
Attachment #467428 - Flags: review?(jdaggett)
Attachment #467428 - Flags: review?(jdaggett) → review+
You need to log in before you can comment on or make changes to this bug.