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

Status: RESOLVED FIXED

(Core :: Graphics: Text, defect, P3)

Description

[Tyson Smith [:tsmith]]

Attachment: 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

[Behdad Esfahbod]

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.

Comment 2

[Tyson Smith [:tsmith]]

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!"

Comment 3

[Jonathan Kew [:jfkthame]]

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;
   }

Comment 4

[Tyson Smith [:tsmith]]

(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

[Behdad Esfahbod]

Thanks Jonathan.  Can you send a PR upstream?  Thanks.

Comment 6

[Ryan VanderMeulen [:RyanVM]]

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.