bug#41125: 28.0.50; Fwindow_text_pixel_size uses FETCH_CHAR (charpos)

bug#41125: 28.0.50; Fwindow_text_pixel_size uses FETCH_CHAR (charpos)

[Pip Cet]

Fwindow_text_pixel_size contains this code:

      start = pos = BEGV;
      while ((pos++ < ZV) && (c = FETCH_CHAR (pos))
         && (c == ' ' || c == '\t' || c == '\n' || c == '\r'))
    start = pos;
      while ((pos-- > BEGV) && (c = FETCH_CHAR (pos)) && (c == ' ' ||
c == '\t'))
    start = pos;

which cannot possibly be correct: FETCH_CHAR takes a byte position,
not a character position, but BEGV and ZV are measured in characters.

(I'm familiarizing myself with the xdisp.c code, partly by replacing
bytepos/charpos pairs of variables with a combined pos_t type
variable. I think that's a good idea partly because it would prevent
precisely this kind of bug.)

bug#41125: 28.0.50; Fwindow_text_pixel_size uses FETCH_CHAR (charpos)

[Eli Zaretskii]

> From: Pip Cet <pipcet@gmail.com>
> Date: Thu, 7 May 2020 11:55:07 +0000
> 
> Fwindow_text_pixel_size contains this code:
> 
>       start = pos = BEGV;
>       while ((pos++ < ZV) && (c = FETCH_CHAR (pos))
>          && (c == ' ' || c == '\t' || c == '\n' || c == '\r'))
>     start = pos;
>       while ((pos-- > BEGV) && (c = FETCH_CHAR (pos)) && (c == ' ' ||
> c == '\t'))
>     start = pos;
> 
> which cannot possibly be correct: FETCH_CHAR takes a byte position,
> not a character position, but BEGV and ZV are measured in characters.

Ouch!  Thanks.  Does the patch below look good?

> (I'm familiarizing myself with the xdisp.c code, partly by replacing
> bytepos/charpos pairs of variables with a combined pos_t type
> variable. I think that's a good idea partly because it would prevent
> precisely this kind of bug.)

It might be good for a development build, but not for production:
walking the buffer must be very efficient.

diff --git a/src/xdisp.c b/src/xdisp.c
index 19f4f32..c15dd47 100644
--- a/src/xdisp.c
+++ b/src/xdisp.c
@@ -10442,7 +10442,7 @@ height (excluding the height of the mode- or header-line, if any) that
   struct buffer *b;
   struct it it;
   struct buffer *old_b = NULL;
-  ptrdiff_t start, end, pos;
+  ptrdiff_t start, end, bpos;
   struct text_pos startp;
   void *itdata = NULL;
   int c, max_x = 0, max_y = 0, x = 0, y = 0;
@@ -10457,32 +10457,56 @@ height (excluding the height of the mode- or header-line, if any) that
     }
 
   if (NILP (from))
-    start = BEGV;
+    {
+      start = BEGV;
+      bpos = BEGV_BYTE;
+    }
   else if (EQ (from, Qt))
     {
-      start = pos = BEGV;
-      while ((pos++ < ZV) && (c = FETCH_CHAR (pos))
-	     && (c == ' ' || c == '\t' || c == '\n' || c == '\r'))
-	start = pos;
-      while ((pos-- > BEGV) && (c = FETCH_CHAR (pos)) && (c == ' ' || c == '\t'))
-	start = pos;
+      start = BEGV;
+      bpos = BEGV_BYTE;
+      while (bpos < ZV_BYTE)
+	{
+	  FETCH_CHAR_ADVANCE (c, start, bpos);
+	  if (!(c == ' ' || c == '\t' || c == '\n' || c == '\r'))
+	    break;
+	}
+      while (bpos > BEGV_BYTE)
+	{
+	  DEC_BOTH (start, bpos);
+	  c = FETCH_CHAR (bpos);
+	  if (!(c == ' ' || c == '\t'))
+	    break;
+	}
     }
   else
     {
       CHECK_FIXNUM_COERCE_MARKER (from);
       start = min (max (XFIXNUM (from), BEGV), ZV);
+      bpos = CHAR_TO_BYTE (start);
     }
 
+  SET_TEXT_POS (startp, start, bpos);
+
   if (NILP (to))
     end = ZV;
   else if (EQ (to, Qt))
     {
-      end = pos = ZV;
-      while ((pos-- > BEGV) && (c = FETCH_CHAR (pos))
-	     && (c == ' ' || c == '\t' || c == '\n' || c == '\r'))
-	end = pos;
-      while ((pos++ < ZV) && (c = FETCH_CHAR (pos)) && (c == ' ' || c == '\t'))
-	end = pos;
+      end = ZV;
+      bpos = ZV_BYTE;
+      while (bpos > BEGV_BYTE)
+	{
+	  DEC_BOTH (end, bpos);
+	  c = FETCH_CHAR (bpos);
+	  if (!(c == ' ' || c == '\t' || c == '\n' || c == '\r'))
+	    break;
+	}
+      while (bpos < ZV_BYTE)
+	{
+	  FETCH_CHAR_ADVANCE (c, end, bpos);
+	  if (!(c == ' ' || c == '\t'))
+	    break;
+	}
     }
   else
     {
@@ -10499,7 +10523,6 @@ height (excluding the height of the mode- or header-line, if any) that
     max_y = XFIXNUM (y_limit);
 
   itdata = bidi_shelve_cache ();
-  SET_TEXT_POS (startp, start, CHAR_TO_BYTE (start));
   start_display (&it, w, startp);
   /* It makes no sense to measure dimensions of region of text that
      crosses the point where bidi reordering changes scan direction.

bug#41125: 28.0.50; Fwindow_text_pixel_size uses FETCH_CHAR (charpos)

[martin rudalics]

 >> which cannot possibly be correct: FETCH_CHAR takes a byte position,
 >> not a character position, but BEGV and ZV are measured in characters.
 >
 > Ouch!  Thanks.  Does the patch below look good?

It does.  Thanks for fixing yet another of my blunders in this function.
And thanks to Pip for catching it.

martin

bug#41125: 28.0.50; Fwindow_text_pixel_size uses FETCH_CHAR (charpos)

[Eli Zaretskii]

> Cc: 41125@debbugs.gnu.org
> From: martin rudalics <rudalics@gmx.at>
> Date: Fri, 8 May 2020 09:18:27 +0200
> 
>  >> which cannot possibly be correct: FETCH_CHAR takes a byte position,
>  >> not a character position, but BEGV and ZV are measured in characters.
>  >
>  > Ouch!  Thanks.  Does the patch below look good?
> 
> It does.  Thanks for fixing yet another of my blunders in this function.
> And thanks to Pip for catching it.

Thanks, I pushed the fix to the emacs-27 branch.