[harfbuzz] reference binding to null pointer of type 'const hb_ot_map_t::lookup_map_t' [@hb_ot_map_t::get_stage_lookups]

RESOLVED FIXED in Firefox 54

Status

()

Core
Graphics: Text
P3
critical
RESOLVED FIXED
10 months ago
26 days ago

People

(Reporter: tsmith, Unassigned)

Tracking

(Blocks: 1 bug, {csectype-nullptr, sec-other, testcase})

Trunk
mozilla55
csectype-nullptr, sec-other, testcase
Points:
---

Firefox Tracking Flags

(firefox54 fixed)

Details

(Whiteboard: [gfx-noted])

Attachments

(1 attachment)

335 bytes, application/x-font-ttf
Details
(Reporter)

Description

10 months ago
Created attachment 8833506 [details]
test_case.ttf

Found while fuzzing harfbuzz commit 4ec19319ab195d852708661e12da2a6485fce544

This bug requires building with Undefined Behavior Sanitizer (UBSan) to detect.

This bug blocks further fuzzing with UBSan enabled builds. I am marking this bug sec-sensitive because it seems harfbuzz has not been fuzzed with UBSan.

STR:
1) run following command: ./hb-fuzzer test_case.ttf

hb-private.hh:381:66: runtime error: reference binding to null pointer of type 'const hb_ot_map_t::lookup_map_t'
    #0 0x5843d0 in hb_ot_map_t::get_stage_lookups(unsigned int, unsigned int, hb_ot_map_t::lookup_map_t const**, unsigned int*) const (hb-fuzzer+0x5843d0)
    #1 0x56fe15 in data_create_indic(hb_ot_shape_plan_t const*) (hb-fuzzer+0x56fe15)
    #2 0x548b4f in _hb_ot_shaper_shape_plan_data_create (hb-fuzzer+0x548b4f)
    #3 0x525e23 in hb_shape_plan_create2 (hb-fuzzer+0x525e23)
    #4 0x528e4b in hb_shape_plan_create_cached2 (hb-fuzzer+0x528e4b)
    #5 0x5252ba in hb_shape_full (hb-fuzzer+0x5252ba)
    #6 0x4ede24 in LLVMFuzzerTestOneInput (hb-fuzzer+0x4ede24)
    #7 0x4eec08 in main (hb-fuzzer+0x4eec08)
    #8 0x7f58cbb9682f in __libc_start_main /build/glibc-t3gR2i/glibc-2.23/csu/../csu/libc-start.c:291
    #9 0x4197d8 in _start (hb-fuzzer+0x4197d8)

Comment 1

10 months ago
This is harmless. I don't know hot to silence it or improve the code without making it ugly.  In C, it's defined to do "&array[len]" if the array is of length len.  Ie. taking the address of the first element past the end of array is defined.  I'm doing the same here, but array is NULL and len is zero.  It happens that it's an overloaded indexing operator.
(Reporter)

Comment 2

10 months ago
Perhaps this may help:
https://clang.llvm.org/docs/UndefinedBehaviorSanitizer.html#issue-suppression

It does come with the warning: "UndefinedBehaviorSanitizer is not expected to produce false positives. If you see one, look again; most likely it is a true positive!"
Does it help to do this? (Or am I looking at the wrong line altogether?)

diff --git a/src/hb-ot-map-private.hh b/src/hb-ot-map-private.hh
index 0395c9c..b69e534 100644
--- a/src/hb-ot-map-private.hh
+++ b/src/hb-ot-map-private.hh
@@ -113,7 +113,7 @@ struct hb_ot_map_t
     assert (stage <= stages[table_index].len);
     unsigned int start = stage ? stages[table_index][stage - 1].last_lookup : 0;
     unsigned int end   = stage < stages[table_index].len ? stages[table_index][stage].last_lookup : lookups[table_index].len;
-    *plookups = &lookups[table_index][start];
+    *plookups = end == start ? NULL : &lookups[table_index][start];
     *lookup_count = end - start;
   }
(Reporter)

Comment 4

10 months ago
(In reply to Jonathan Kew (:jfkthame) from comment #3)
> Does it help to do this? (Or am I looking at the wrong line altogether?)
> 
> diff --git a/src/hb-ot-map-private.hh b/src/hb-ot-map-private.hh
> index 0395c9c..b69e534 100644
> --- a/src/hb-ot-map-private.hh
> +++ b/src/hb-ot-map-private.hh
> @@ -113,7 +113,7 @@ struct hb_ot_map_t
>      assert (stage <= stages[table_index].len);
>      unsigned int start = stage ? stages[table_index][stage - 1].last_lookup
> : 0;
>      unsigned int end   = stage < stages[table_index].len ?
> stages[table_index][stage].last_lookup : lookups[table_index].len;
> -    *plookups = &lookups[table_index][start];
> +    *plookups = end == start ? NULL : &lookups[table_index][start];
>      *lookup_count = end - start;
>    }

I tested this patch and it removes the undefined behavior.

Comment 5

10 months ago
Thanks Jonathan.  Can you send a PR upstream?  Thanks.
Whiteboard: [gfx-noted]
Priority: -- → P3
This was fixed by the below upstream commit, which was included in harfbuzz 1.4.6.
https://github.com/behdad/harfbuzz/commit/740fdbcd0e6d25c1d6f12537ca2aa559650b9d52

We updated to version 1.4.6 in bug 1358502 for Fx55 and also backported it to Fx54.
Status: NEW → RESOLVED
Last Resolved: a month ago
status-firefox54: affected → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla55
Group: gfx-core-security → core-security-release
Group: core-security-release
You need to log in before you can comment on or make changes to this bug.