[HarfBuzz] GCC 12 warning of use after free in hbfont.cc

[Behdad Esfahbod]

Hi phil,

The analysis is correct. The only part you / gcc are missing is
that hb_font_funcs_set_glyph_func() starts with:

  if (hb_object_is_immutable (ffuncs))
  {
    if (destroy)
      destroy (user_data);
    return;
  }

So we know ffuncs is not immutable.

At any rate, I moved the code around, so the compiler should not be
confused anymore:


https://github.com/harfbuzz/harfbuzz/commit/b58bfd9818243fc1178ecad0731fa24a5aa3f235

Hope that helps,

behdad
http://behdad.org/


On Wed, May 11, 2022 at 8:32 PM Philip Race <philip.race at oracle.com> wrote:

> Someone tried building OpenJDK (which includes harfbuzz) using gcc 12
> and got this warning
>
> In function 'void trampoline_reference(hb_trampoline_closure_t*)',
>      inlined from 'void hb_font_funcs_set_glyph_func(hb_font_funcs_t*,
> hb_font_get_glyph_func_t, void*, hb_destroy_func_t)' at
>
> /home/ysuenaga/github-forked/jdk/src/java.desktop/share/native/libharfbuzz/hb-font.cc:2368:24:
> /home/ysuenaga/github-forked/jdk/src/java.desktop/share/native/libharfbuzz/hb-font.cc:2286:12:
>
> error: pointer 'trampoline' used after 'void free(void*)'
> [-Werror=use-after-free]
>   2286 |   closure->ref_count++;
>        |   ~~~~~~~~~^~~~~~~~~
>      inlined from 'void trampoline_destroy(void*)' at
>
> /home/ysuenaga/github-forked/jdk/src/java.desktop/share/native/libharfbuzz/hb-font.cc:2290:1,
>      inlined from 'void
> hb_font_funcs_set_nominal_glyph_func(hb_font_funcs_t*,
> hb_font_get_nominal_glyph_func_t, void*, hb_destroy_func_t)' at
>
> /home/ysuenaga/github-forked/jdk/src/java.desktop/share/native/libharfbuzz/hb-font.cc:733:1,
>      inlined from 'void hb_font_funcs_set_glyph_func(hb_font_funcs_t*,
> hb_font_get_glyph_func_t, void*, hb_destroy_func_t)' at
>
> /home/ysuenaga/github-forked/jdk/src/java.desktop/share/native/libharfbuzz/hb-font.cc:2363:40:
> /home/ysuenaga/github-forked/jdk/src/java.desktop/share/native/libharfbuzz/hb-font.cc:2299:8:
>
> note: call to 'void free(void*)' here
>   2299 |   free (closure);
>
> The line numbers don't match current git sources since OpenJDK has an
> older version but I think upstream is the same in this regard.
>
> It is referring to this code
> ----------
>   hb_font_funcs_set_nominal_glyph_func (ffuncs,
> hb_font_get_nominal_glyph_trampoline,
>                                          trampoline,
>                                          trampoline_destroy);
>
>    trampoline_reference (&trampoline->closure);
> --------
> I *think* the potential problem is this
> void \
> hb_font_funcs_set_##name##_func (hb_font_funcs_t *ffuncs,    \
>                                   hb_font_get_##name##_func_t func,      \
>                                   void *user_data, \
>                                   hb_destroy_func_t destroy)   \
> { \
>    if (hb_object_is_immutable (ffuncs))                                   \
> { \
>      if (destroy)                                                         \
>        destroy (user_data);                                               \
> return; \
>    }
>
> The macro expansion of that is hb_font_funcs_set_nominal_glyph_func
> which is called from
> hb_font_funcs_set_glyph_func and on return if that
> hb_object_is_immutable code path was taken
> then the trampoline reference will have been freed already since we have
>
> static void
> trampoline_destroy (void *user_data)
> {
>    hb_trampoline_closure_t *closure = (hb_trampoline_closure_t *)
> user_data;
>
>    if (--closure->ref_count)
>      return;
>
>    if (closure->destroy)
>      closure->destroy (closure->user_data);
>    free (closure);
> }
>
> I suppose if it is never immutable no problem, but I don't know how you
> prove that.
> And it isn't clear what fix would be wanted.
>
>
> -phil
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://lists.freedesktop.org/archives/harfbuzz/attachments/20220516/cba6ba5c/attachment.htm>