Segmentation fault with empty buffer

[simoncozens]

This file:

#include <ft2build.h>
#include <assert.h>
#include <hb.h>
#include <hb-ft.h>

int main() {
  hb_segment_properties_t segment_props;
  hb_shape_plan_t *shape_plan;
  hb_buffer_t *buf;
  FT_Library ft_library = malloc(sizeof(FT_Library));
  FT_Init_FreeType(&ft_library);

  FT_Face face = (FT_Face)malloc(sizeof(FT_Face));
  char text[] = "";
  assert(!FT_New_Face(ft_library, "/Users/simon/Library/Fonts/Gentium-R.ttf", 0, &face));
  hb_face_t* hbFace = hb_ft_face_create_cached(face);
  hb_font_t* hb_ft_font = hb_font_create (hbFace);
  hb_ft_font_set_funcs(hb_ft_font);
  buf = hb_buffer_create();
  hb_buffer_add_utf8(buf, text, strlen(text), 0, strlen(text));
  hb_buffer_guess_segment_properties(buf);
  hb_buffer_get_segment_properties(buf, &segment_props);
  shape_plan = hb_shape_plan_create_cached(hbFace, &segment_props, NULL, 0, NULL);
  int res = hb_shape_plan_execute(shape_plan, hb_ft_font, buf, NULL, 0);
}

produces a segfault here:

* thread #1: tid = 0x17b90, 0x000000010004b700 libharfbuzz.0.dylib`_hb_ot_shape_fallback_position_recategorize_marks(hb_ot_shape_plan_t const*, hb_font_t*, hb_buffer_t*) [inlined] _hb_glyph_info_get_general_category(hb_glyph_info_t const*) + 7 at hb-ot-layout-private.hh:245, queue = 'com.apple.main-thread', stop reason = EXC_BAD_ACCESS (code=1, address=0x10)
  * frame #0: 0x000000010004b700 libharfbuzz.0.dylib`_hb_ot_shape_fallback_position_recategorize_marks(hb_ot_shape_plan_t const*, hb_font_t*, hb_buffer_t*) [inlined] _hb_glyph_info_get_general_category(hb_glyph_info_t const*) + 7 at hb-ot-layout-private.hh:245
    frame #1: 0x000000010004b6f9 libharfbuzz.0.dylib`_hb_ot_shape_fallback_position_recategorize_marks(plan=0x0000000100812800, font=0x0000000100305420, buffer=<unavailable>) + 105 at hb-ot-shape-fallback.cc:172
    frame #2: 0x000000010003bc59 libharfbuzz.0.dylib`::_hb_ot_shape(hb_shape_plan_t *, hb_font_t *, hb_buffer_t *, const hb_feature_t *, unsigned int) [inlined] hb_ot_substitute_default(hb_ot_shape_context_t*) + 1407 at hb-ot-shape.cc:567
    frame #3: 0x000000010003b6da libharfbuzz.0.dylib`::_hb_ot_shape(hb_shape_plan_t *, hb_font_t *, hb_buffer_t *, const hb_feature_t *, unsigned int) [inlined] hb_ot_substitute(hb_ot_shape_context_t*) at hb-ot-shape.cc:595
    frame #4: 0x000000010003b6da libharfbuzz.0.dylib`::_hb_ot_shape(hb_shape_plan_t *, hb_font_t *, hb_buffer_t *, const hb_feature_t *, unsigned int) [inlined] hb_ot_shape_internal(hb_ot_shape_context_t*) + 1519 at hb-ot-shape.cc:795
    frame #5: 0x000000010003b0eb libharfbuzz.0.dylib`::_hb_ot_shape(shape_plan=<unavailable>, font=0x0000000100305420, buffer=0x0000000100306cd0, features=<unavailable>, num_features=<unavailable>) + 59 at hb-ot-shape.cc:816
    frame #6: 0x00000001000115d4 libharfbuzz.0.dylib`::hb_shape_plan_execute(shape_plan=0x0000000100306e60, font=0x0000000100305420, buffer=0x0000000100306cd0, features=0x0000000000000000, num_features=0) + 356 at hb-shaper-list.hh:43
    frame #7: 0x0000000100000e05 test`main + 453 at test.c:24

[behdad]

Fixed with 26ba4d1

[behdad]

Actually, can you debug why you were even getting into this loop to begin with:

void 
_hb_ot_shape_fallback_position_recategorize_marks (const hb_ot_shape_plan_t *plan HB_UNUSED, 
↦       ↦       ↦       ↦       ↦       ↦          hb_font_t *font HB_UNUSED, 
↦       ↦       ↦       ↦       ↦       ↦          hb_buffer_t  *buffer) 
{ 
  unsigned int count = buffer->len; 
  hb_glyph_info_t *info = buffer->info; 
  for (unsigned int i = 0; i < count; i++) 
    if (_hb_glyph_info_get_general_category (&info[i]) == HB_UNICODE_GENERAL_CATEGORY_NON_SPACING_MARK) { 
      unsigned int combining_class = _hb_glyph_info_get_modified_combining_class (&info[i]); 
      combining_class = recategorize_combining_class (info[i].codepoint, combining_class); 
      _hb_glyph_info_set_modified_combining_class (&info[i], combining_class); 
    } 
} 

if the buffer len is zero, that shouldn't happen.

[simoncozens]

Buffer len is turning to 1, somewhere inside _hb_ot_shape_normalize (c->plan, buffer, c->font);

[simoncozens]

Specifically, the very final buffer->swap_buffers (); means that buffer now has a length of 1.

[simoncozens]

buffer->next_glyph is called unconditionally. Maybe that's it?

[behdad]

Humm. Ouch. Ok, I'll take a look. In general we wrote most of the code assuming that buffer is NOT empty.

Currently we guarantee that if the buffer started unempty, it would always remain unempty. But that will change at some point because of this comment in hb-ot-layout-gsub-table.hh:Sequence::apply():

    /* TODO: 
     * Testing shows that Uniscribe actually allows zero-len susbstitute, 
     * which essentially deletes a glyph.  We don't allow for now.  It 
     * can be confusing to the client since the cluster from the deleted 
     * glyph won't be merged with any output cluster...  Also, currently 
     * buffer->move_to() makes assumptions about this too.  Perhaps fix 
     * in the future after figuring out what to do with the clusters. 
     */ 

So, I like to remove such assumptions... Perhaps I should remove the "if (!buffer->len) return true" such that we exercise more codepaths with empty buffers and fix broken ones.

cc @jfkthame

[behdad]

buffer->next_glyph is called unconditionally. Maybe that's it?

Sounds like it.

[behdad]

Fixed. Thanks.