GNU bug report logs - #41125
28.0.50; Fwindow_text_pixel_size uses FETCH_CHAR (charpos)

Previous Next

To add a comment to this bug, you must first unarchive it, by sending
a message to control AT debbugs.gnu.org, with unarchive 41125 in the body.
You can then email your comments to 41125 AT debbugs.gnu.org in the normal way.

Toggle the display of automated, internal messages from the tracker.

Message #5 received at submit <at> debbugs.gnu.org (full text, mbox):

From: Pip Cet <pipcet <at> gmail.com>
To: bug-gnu-emacs <at> gnu.org
Subject: 28.0.50; Fwindow_text_pixel_size uses FETCH_CHAR (charpos)
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.

(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.)

Message #8 received at 41125 <at> debbugs.gnu.org (full text, mbox):

From: Eli Zaretskii <eliz <at> gnu.org>
To: Pip Cet <pipcet <at> gmail.com>, martin rudalics <rudalics <at> gmx.at>
Cc: 41125 <at> debbugs.gnu.org
Subject: Re: bug#41125: 28.0.50;
 Fwindow_text_pixel_size uses FETCH_CHAR (charpos)
Date: Thu, 07 May 2020 16:36:19 +0300

> From: Pip Cet <pipcet <at> 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.

Message #11 received at 41125 <at> debbugs.gnu.org (full text, mbox):

From: martin rudalics <rudalics <at> gmx.at>
To: Eli Zaretskii <eliz <at> gnu.org>, Pip Cet <pipcet <at> gmail.com>
Cc: 41125 <at> debbugs.gnu.org
Subject: Re: bug#41125: 28.0.50; Fwindow_text_pixel_size uses FETCH_CHAR
 (charpos)
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.

martin

Message #16 received at 41125-done <at> debbugs.gnu.org (full text, mbox):

From: Eli Zaretskii <eliz <at> gnu.org>
To: martin rudalics <rudalics <at> gmx.at>
Cc: 41125-done <at> debbugs.gnu.org, pipcet <at> gmail.com
Subject: Re: bug#41125: 28.0.50; Fwindow_text_pixel_size uses FETCH_CHAR
 (charpos)
Date: Fri, 08 May 2020 13:37:42 +0300

> Cc: 41125 <at> debbugs.gnu.org
> From: martin rudalics <rudalics <at> 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.

Previous Next

GNU bug tracking system
Copyright (C) 1999 Darren O. Benham, 1997,2003 nCipher Corporation Ltd, 1994-97 Ian Jackson.