1296024 - [harfbuzz] Assertion `end == match_positions[idx]' failed [@OT::apply_lookup]

Status: RESOLVED FIXED

(Core :: Graphics: Text, defect)

Description

[Tyson Smith [:tsmith]]

Attachment: log.txt

Found while fuzzing harfbuzz revision 18c19dd34dcdcaab0a6d47768339f8fb70c0d3f0

hb-fuzzer: hb-ot-layout-gsubgpos-private.hh:1003: bool OT::apply_lookup(OT::hb_apply_context_t*, unsigned int, unsigned int*, unsigned int, const OT::LookupRecord*, unsigned int): Assertion `end == match_positions[idx]' failed.

Program received signal SIGABRT, Aborted.
0x00007ffff719a418 in __GI_raise (sig=sig@entry=6) at ../sysdeps/unix/sysv/linux/raise.c:54
54	../sysdeps/unix/sysv/linux/raise.c: No such file or directory.
(gdb) bt
#0  0x00007ffff719a418 in __GI_raise (sig=sig@entry=6) at ../sysdeps/unix/sysv/linux/raise.c:54
#1  0x00007ffff719c01a in __GI_abort () at abort.c:89
#2  0x00007ffff7192bd7 in __assert_fail_base (fmt=<optimized out>, 
    assertion=assertion@entry=0x44c139 "end == match_positions[idx]", 
    file=file@entry=0x44c1d8 "hb-ot-layout-gsubgpos-private.hh", line=line@entry=1003, 
    function=function@entry=0x44c500 <OT::apply_lookup(OT::hb_apply_context_t*, unsigned int, unsigned int*, unsigned int, OT::LookupRecord const*, unsigned int)::__PRETTY_FUNCTION__> "bool OT::apply_lookup(OT::hb_apply_context_t*, unsigned int, unsigned int*, unsigned int, const OT::LookupRecord*, unsigned int)") at assert.c:92
#3  0x00007ffff7192c82 in __GI___assert_fail (
    assertion=assertion@entry=0x44c139 "end == match_positions[idx]", 
    file=file@entry=0x44c1d8 "hb-ot-layout-gsubgpos-private.hh", line=line@entry=1003, 
    function=function@entry=0x44c500 <OT::apply_lookup(OT::hb_apply_context_t*, unsigned int, unsigned int*, unsigned int, OT::LookupRecord const*, unsigned int)::__PRETTY_FUNCTION__> "bool OT::apply_lookup(OT::hb_apply_context_t*, unsigned int, unsigned int*, unsigned int, const OT::LookupRecord*, unsigned int)") at assert.c:101
#4  0x0000000000425d43 in OT::apply_lookup (c=0x7fffffffd9d0, count=<optimized out>, 
    match_positions=0x7fffffffd4b0, lookupCount=<optimized out>, lookupRecord=<optimized out>, 
    match_length=<optimized out>) at hb-ot-layout-gsubgpos-private.hh:1003
...
Full log is attached.

Comment 1

[Tyson Smith [:tsmith]]

Attachment: test_case.ttf

Comment 2

[Daniel Veditz [:dveditz]]

I'm guessing you're fuzzing harfbuzz directly; does the Open Type Sanitizer reject this font if you try to use it in Firefox? That would affect the severity rating (for mozilla).

Comment 3

[Jonathan Kew [:jfkthame]]

OTS doesn't validate the OpenType Layout tables that harfbuzz uses; it passes them through as-is, because harfbuzz performs its own sanitization.

Comment 4

[Daniel Veditz [:dveditz]]

Tyson: does this happen in the in-tree version of harfbuzz, or only the upstream?

Behdad: is this benign (handled)? Immediately after the assert there's a break and then the value isn't used until the moveto at the end. The condition before the assert is end <= the index so if the assert fires end is less than we think which sounds better than being too big -- but I don't know what happens with the buffer after that.

Comment 5

[Behdad Esfahbod]

Ok, my bad, from the commit hash looks like this is current.  I'll take a look soonish.

Comment 6

[Tyson Smith [:tsmith]]

(In reply to Daniel Veditz [:dveditz] from comment #2)
> I'm guessing you're fuzzing harfbuzz directly; does the Open Type Sanitizer
> reject this font if you try to use it in Firefox? That would affect the
> severity rating (for mozilla).

This test case will likely not affect Firefox even if the vulnerable code is in the browser because this test case has been reduced. So it will likely look invalid to anything other than the tool it was found with (hb-fuzzer in this case).

I'll check the in-tree version and post and update.

Comment 7

[Tyson Smith [:tsmith]]

(In reply to Daniel Veditz [:dveditz] from comment #4)
> Tyson: does this happen in the in-tree version of harfbuzz, or only the
> upstream?
> 
> Behdad: is this benign (handled)? Immediately after the assert there's a
> break and then the value isn't used until the moveto at the end. The
> condition before the assert is end <= the index so if the assert fires end
> is less than we think which sounds better than being too big -- but I don't
> know what happens with the buffer after that.

According to https://dxr.mozilla.org/mozilla-central/source/gfx/harfbuzz/README-mozilla
We are including commit 547ddb0...c5f82 from Aug 18th which will be affected by this issue.

Comment 8

[Daniel Veditz [:dveditz]]

Behdad: did you get a chance to look at this? (comment 5)

Comment 9

[Al Billings [:abillings - ex-MoCo]]

Tyson, is this still reproducing?

Comment 10

[Tyson Smith [:tsmith]]

(In reply to Al Billings [:abillings] from comment #9)
> Tyson, is this still reproducing?

Yes

Comment 11

[Tyson Smith [:tsmith]]

(In reply to Tyson Smith [:tsmith] from comment #10)
> (In reply to Al Billings [:abillings] from comment #9)
> > Tyson, is this still reproducing?
> 
> Yes

Scratch that sorry. I was looking at the wrong bug. I'll have a look.

Comment 12

[Behdad Esfahbod]

I believe I fixed this in 1.4.0

Comment 13

[Tyson Smith [:tsmith]]

I can confirm this is in fact no longer reproducible.

Comment 14

[Stephanie Ouillon [:arroway] (needinfo me)]

I understand we can close this bug?

Comment 15

[Mats Palmgren (inactive)]

The latest update was to harfbuzz 1.4.2 (bug 1333656) for v54, and 1.4.1 (bug 1325775) for v53.
Behdad says he believes he fixed this in 1.4.0 (comment 12) and since it stopped being
reproducible (comment 13) that seems to be the case.  So I'd say v53/v54 is fixed.

We're using harfbuzz 1.3.3 in Beta (v52) so I assume v52 and older are still affected.

Comment 16

[Daniel Veditz [:dveditz]]

Tyson: Did you ever check the in-tree version (comment 6)?

Comment 17

[Daniel Veditz [:dveditz]]

(I ask because we may need to fix this on 52 and ESR-45, either by upgrading harfbuzz or cherrypicking Behdad's patch)

Comment 18

[Tyson Smith [:tsmith]]

(In reply to Daniel Veditz [:dveditz] from comment #16)
> Tyson: Did you ever check the in-tree version (comment 6)?

This is reproducible in 1.3.3 and older. I have also verified the current in-tree version 1.4.1 is fixed.

Comment 19

[Ryan VanderMeulen [:RyanVM]]

To close the loop here, I'll request Beta/ESR45 approval on the 1.4.1 update over in bug 1325775.

Comment 20

[Ritu Kothari (:ritu) (Inactive, please n-i to RyanVM, jcristau, or pascal)]

Hi Dan, is there something we need to uplift to Beta52 for this? Who might be able to help?

Comment 21

[Ryan VanderMeulen [:RyanVM]]

It was done over in bug 1325775.

Comment 22

[Ryan VanderMeulen [:RyanVM]]

Behdad, our version of HarfBuzz on ESR45 is way too old to comfortably update to version 1.4.1. Can you please point me to the upstream commit(s) that resolved this so we can look into cherry-picking instead? Thanks!

Comment 23

[Behdad Esfahbod]

(In reply to Ryan VanderMeulen [:RyanVM] from comment #22)
> Behdad, our version of HarfBuzz on ESR45 is way too old to comfortably
> update to version 1.4.1. Can you please point me to the upstream commit(s)
> that resolved this so we can look into cherry-picking instead? Thanks!

If your version is way too old, it's probably way too much work to list all the relevant commits...
This particular one was 359dead96, but I don't vouch for a way-too-old HB plus this patch.

Comment 24

[Ryan VanderMeulen [:RyanVM]]

Attachment: cherry-picked patch for esr45

Understood, thanks Behdad! That change actually applies cleanly to the version included with ESR45, so that's nice anyway :)

That said, Tyson was unable to reproduce this assertion in the browser, so I'm not sure what we should even do here. Jonathan, should we take this for belts and suspenders' sake or just wontfix for esr45?

Comment 25

[Behdad Esfahbod]

That's because hb-fuzzer puts smaller limits on buffer expansion factors before further expansion is declined.  As such, what hits a bug in hb-fuzzer does not reproduce in production builds.  But it's possible to create fonts to hit it in production as well, just bigger.

Comment 26

[Jonathan Kew [:jfkthame]]

Comment on attachment 8836289 [details] [diff] [review]
cherry-picked patch for esr45

Review of attachment 8836289 [details] [diff] [review]:
-----------------------------------------------------------------

This looks like the commit that -introduced- that assertion, rather than fixed it; it was later removed in 4b4a1b9f235598b04ce9ae1f9670fc978ab7620d.

AFAICS, esr45 is on a much older harfbuzz version (1.1.0 or thereabouts), where this isn't present at all. (So that version still suffers from bug 1128658, which is what https://github.com/behdad/harfbuzz/issues/253 was about, and doubtless various other issues, but not the assertion issue here.)

Comment 27

[Ryan VanderMeulen [:RyanVM]]

I'm calling this wontfix for esr45 then (potentially even unaffected would have been better). But either way, this feels like more backport trouble than it's worth. Feel free to change it back if you feel otherwise, Jonathan.

Comment 28

[Jonathan Kew [:jfkthame]]

I'm fine with that.