Theoretical division by zero in hb-ot-shape-complex-arabic.cc apply_stch()

[bnason-nf]

Hi,

I ran the clang static analyzer on harfbuzz 1.4.2 and it reported this potential issue:

Logic error: Division by zero (harfbuzz/src/hb-ot-shape-complex-arabic.cc:548)
harfbuzz/src/hb-ot-shape-complex-arabic.cc

   534          int n_copies = 0;
   535    
   536          hb_position_t w_remaining = w_total - w_fixed;
   537          if (sign * w_remaining > sign * w_repeating && sign * w_repeating > 0)
   538    	n_copies = (sign * w_remaining) / (sign * w_repeating) - 1;
   539    
   540          /* See if we can improve the fit by adding an extra repeat and squeezing them together a bit. */
   541          hb_position_t extra_repeat_overlap = 0;
   542          hb_position_t shortfall = sign * w_remaining - sign * w_repeating * (n_copies + 1);
   543          if (shortfall > 0)
   544          {
   545            ++n_copies;
   546            hb_position_t excess = (n_copies + 1) * sign * w_repeating - sign * w_remaining;
   547            if (excess > 0)
   548              extra_repeat_overlap = excess / (n_copies * n_repeating);
                                                  ^ Logic error: Division by zero
   549          }
   550    
   551          if (step == MEASURE)
   552          {
   553    	extra_glyphs_needed += n_copies * n_repeating;
   554    	DEBUG_MSG (ARABIC, NULL, "will add extra %d copies of repeating tiles", n_copies);
   555          }
   556          else
   557          {
   558    	hb_position_t x_offset = 0;
   559    	for (unsigned int k = end; k > start; k--)
   560    	{
   561    	  hb_position_t width = font->get_glyph_h_advance (info[k - 1].codepoint);
   562    
 
Entering loop body
harfbuzz/src/hb-ot-shape-complex-arabic.cc

   467      unsigned int extra_glyphs_needed = 0; // Set during MEASURE, used during CUT
   468      typedef enum { MEASURE, CUT } step_t;
   469    
   470      for (step_t step = MEASURE; step <= CUT; step = (step_t) (step + 1))
                                        ^ Entering loop body
   471      {
   472        unsigned int count = buffer->len;
   473        hb_glyph_info_t *info = buffer->info;
 
Entering loop body
harfbuzz/src/hb-ot-shape-complex-arabic.cc

   474        hb_glyph_position_t *pos = buffer->pos;
   475        unsigned int new_len = count + extra_glyphs_needed; // write head during CUT
   476        unsigned int j = new_len;
   477        for (unsigned int i = count; i; i--)
                                           ^ Entering loop body
   478        {
   479          if (!hb_in_range<unsigned> (info[i - 1].arabic_shaping_action(), STCH_FIXED, STCH_REPEATING))
   480          {
 
Looping back to the head of the loop
harfbuzz/src/hb-ot-shape-complex-arabic.cc

   474        hb_glyph_position_t *pos = buffer->pos;
   475        unsigned int new_len = count + extra_glyphs_needed; // write head during CUT
   476        unsigned int j = new_len;
   477        for (unsigned int i = count; i; i--)
              ^ Looping back to the head of the loop
   478        {
   479          if (!hb_in_range<unsigned> (info[i - 1].arabic_shaping_action(), STCH_FIXED, STCH_REPEATING))
   480          {
 
Looping back to the head of the loop
harfbuzz/src/hb-ot-shape-complex-arabic.cc

   467      unsigned int extra_glyphs_needed = 0; // Set during MEASURE, used during CUT
   468      typedef enum { MEASURE, CUT } step_t;
   469    
   470      for (step_t step = MEASURE; step <= CUT; step = (step_t) (step + 1))
            ^ Looping back to the head of the loop
   471      {
   472        unsigned int count = buffer->len;
   473        hb_glyph_info_t *info = buffer->info;
 
Entering loop body
harfbuzz/src/hb-ot-shape-complex-arabic.cc

   467      unsigned int extra_glyphs_needed = 0; // Set during MEASURE, used during CUT
   468      typedef enum { MEASURE, CUT } step_t;
   469    
   470      for (step_t step = MEASURE; step <= CUT; step = (step_t) (step + 1))
                                        ^ Entering loop body
   471      {
   472        unsigned int count = buffer->len;
   473        hb_glyph_info_t *info = buffer->info;
 
Entering loop body
harfbuzz/src/hb-ot-shape-complex-arabic.cc

   474        hb_glyph_position_t *pos = buffer->pos;
   475        unsigned int new_len = count + extra_glyphs_needed; // write head during CUT
   476        unsigned int j = new_len;
   477        for (unsigned int i = count; i; i--)
                                           ^ Entering loop body
   478        {
   479          if (!hb_in_range<unsigned> (info[i - 1].arabic_shaping_action(), STCH_FIXED, STCH_REPEATING))
   480          {
 
Looping back to the head of the loop
harfbuzz/src/hb-ot-shape-complex-arabic.cc

   474        hb_glyph_position_t *pos = buffer->pos;
   475        unsigned int new_len = count + extra_glyphs_needed; // write head during CUT
   476        unsigned int j = new_len;
   477        for (unsigned int i = count; i; i--)
              ^ Looping back to the head of the loop
   478        {
   479          if (!hb_in_range<unsigned> (info[i - 1].arabic_shaping_action(), STCH_FIXED, STCH_REPEATING))
   480          {
 
Entering loop body
harfbuzz/src/hb-ot-shape-complex-arabic.cc

   474        hb_glyph_position_t *pos = buffer->pos;
   475        unsigned int new_len = count + extra_glyphs_needed; // write head during CUT
   476        unsigned int j = new_len;
   477        for (unsigned int i = count; i; i--)
                                           ^ Entering loop body
   478        {
   479          if (!hb_in_range<unsigned> (info[i - 1].arabic_shaping_action(), STCH_FIXED, STCH_REPEATING))
   480          {
 
Loop body executed 0 times
harfbuzz/src/hb-ot-shape-complex-arabic.cc

   496          int n_repeating = 0;
   497    
   498          unsigned int end = i;
   499          while (i &&
                       ^ Loop body executed 0 times
   500    	     hb_in_range<unsigned> (info[i - 1].arabic_shaping_action(), STCH_FIXED, STCH_REPEATING))
   501          {
   502    	i--;
 
Entering loop body
harfbuzz/src/hb-ot-shape-complex-arabic.cc

   514          }
   515          unsigned int start = i;
   516          unsigned int context = i;
   517          while (context &&
                       ^ Entering loop body
   518    	     !hb_in_range<unsigned> (info[context - 1].arabic_shaping_action(), STCH_FIXED, STCH_REPEATING) &&
   519    	     (_hb_glyph_info_is_default_ignorable (&info[context - 1]) ||
   520    	      HB_ARABIC_GENERAL_CATEGORY_IS_WORD (_hb_glyph_info_get_general_category (&info[context - 1]))))
 
Looping back to the head of the loop
harfbuzz/src/hb-ot-shape-complex-arabic.cc

   514          }
   515          unsigned int start = i;
   516          unsigned int context = i;
   517          while (context &&
                ^ Looping back to the head of the loop
   518    	     !hb_in_range<unsigned> (info[context - 1].arabic_shaping_action(), STCH_FIXED, STCH_REPEATING) &&
   519    	     (_hb_glyph_info_is_default_ignorable (&info[context - 1]) ||
   520    	      HB_ARABIC_GENERAL_CATEGORY_IS_WORD (_hb_glyph_info_get_general_category (&info[context - 1]))))
 
Assuming 'step' is not equal to MEASURE
harfbuzz/src/hb-ot-shape-complex-arabic.cc

   524          }
   525          i++; // Don't touch i again.
   526    
   527          DEBUG_MSG (ARABIC, NULL, "%s stretch at (%d,%d,%d)",
                ^ Assuming 'step' is not equal to MEASURE
   528    		 step == MEASURE ? "measuring" : "cutting", context, start, end);
   529          DEBUG_MSG (ARABIC, NULL, "rest of word:    count=%d width %d", start - context, w_total);
   530          DEBUG_MSG (ARABIC, NULL, "fixed tiles:     count=%d width=%d", n_fixed, w_fixed);
 
Assuming 'shortfall' is > 0
harfbuzz/src/hb-ot-shape-complex-arabic.cc

   540          /* See if we can improve the fit by adding an extra repeat and squeezing them together a bit. */
   541          hb_position_t extra_repeat_overlap = 0;
   542          hb_position_t shortfall = sign * w_remaining - sign * w_repeating * (n_copies + 1);
   543          if (shortfall > 0)
                    ^ Assuming 'shortfall' is > 0
   544          {
   545            ++n_copies;
   546            hb_position_t excess = (n_copies + 1) * sign * w_repeating - sign * w_remaining;
 
Assuming 'excess' is > 0
harfbuzz/src/hb-ot-shape-complex-arabic.cc

   544          {
   545            ++n_copies;
   546            hb_position_t excess = (n_copies + 1) * sign * w_repeating - sign * w_remaining;
   547            if (excess > 0)
                      ^ Assuming 'excess' is > 0
   548              extra_repeat_overlap = excess / (n_copies * n_repeating);
   549          }
   550    
 
Division by zero
harfbuzz/src/hb-ot-shape-complex-arabic.cc

   545            ++n_copies;
   546            hb_position_t excess = (n_copies + 1) * sign * w_repeating - sign * w_remaining;
   547            if (excess > 0)
   548              extra_repeat_overlap = excess / (n_copies * n_repeating);
                                                  ^ Division by zero
   549          }
   550    
   551          if (step == MEASURE)

So basically if n_repeating is zero this could crash, which should be easy to address.

Thanks,
Ben

[jfkthame]

So I guess this could arise if a font has a 'stch' feature where the substitution "decomposes" a glyph to a single glyph (rather than the expected sequence of 3 or even 5 glyphs), so there's no repeating element. That seems like a rather pointless use of the feature, but isn't technically invalid AFAICS.

I'm not sure offhand whether we'd actually reach this code with such a font (or if we'd short-circuit somewhere earlier), but it seems like it'd be worth including a zero-check here to be on the safe side.

[pwithnall]

Coverity also detected this, but I think it’s a false positive. Here’s a copy/paste of my analysis from the Coverity issue:

n_copies can be proven to be non-zero on all code paths up to this point. If n_repeating is zero, that means w_repeating has never been added to, so (w_repeating == 0). We must have (shortfall > 0), so (sign * w_remaining > 0). This means that (excess < 0), which invalidates the if-condition on line 547.

[behdad]

I checked the code and I also think this cannot happen, because excess > 0 implies w_repeating != 0 which implies n_repeating != 0. But happy to take a simple fix to silence the error. Doing so.