bug#41520: 28.0.50; Crash in character.h due to assertion error

[Pip Cet <pipcet@gmail.com>]

[-- Attachment #1: Type: text/plain, Size: 2197 bytes --]

On Mon, May 25, 2020 at 2:18 PM Eli Zaretskii <eliz@gnu.org> wrote:
> > From: Pip Cet <pipcet@gmail.com>
> > Date: Mon, 25 May 2020 07:28:23 +0000
> > Cc: 41520@debbugs.gnu.org
> > My suggestion is to drop the "eassume" on emacs-27, and fix FETCH_CHAR
> > to FETCH_BYTE on master.
>
> There's no eassume on emacs-27, this is new on master.

Yeah, I realized after hitting "Send" that I'd looked at the wrong tree :-)

> So I installed on emacs-27 branch a patch very similar to what you
> sent, except that it uses FETCH_BYTE in all cases where we compare to
> a newline: this is both more efficient and more correct.

I'm attaching a patch with a few more cases. I don't have a strong
preference for either FETCH_BYTE or FETCH_CHAR where both will do, but
we should be consistent.

> > (I'd like to reiterate my proposal to use a pos_t for bytepos/charpos
> > pairs, which would catch or silently fix (which happened in this case
> > on my pos_t branch) such bugs. The code on my branch reads:
> >
> >   else if (POS_CHAR_EQUAL (it->bidi_it.pos, bob)
> >        || (!string_p
> >            && (FETCH_CHAR (dec_pos (it->bidi_it.pos)) == '\n'
> >            || FETCH_CHAR (it->bidi_it.pos) == '\n')))
> >
> > which, while minimally slower, doesn't throw assertion errors.)
>
> That would require us to maintain both character and byte positions
> where we use these macros,

For now, I'd like to introduce them in Emacs proper only where we have
pairs of variables anyway.

> something that could be redundant
> overhead.

It would be redundant to use a pos_t where a ptrdiff_t suffices, yes.
I'm not proposing to do that at present, though I think there are some
places that do charpos/bytepos conversions unnecessarily because only
one of them is passed down the call chain. So, no, it wouldn't be
redundant overhead, and it might actually make some code paths faster.

> Moreover, I think we prefer having assertions in the debug
> builds rather then silent fixups (and in production eassume compiles
> into something that doesn't abort).

I see no way to have assertions, and I think free bug fixes are better
than undiscovered bugs. It's really hard to get things wrong with a
stronger type.

[-- Attachment #2: 0001-Fix-more-single-byte-accesses.patch --]
[-- Type: text/x-patch, Size: 2439 bytes --]

From 702efade7a5bb759f1d6df07e20180ba95d87f3d Mon Sep 17 00:00:00 2001
From: Pip Cet <pipcet@gmail.com>
Date: Mon, 25 May 2020 14:20:46 +0000
Subject: [PATCH] Fix more single-byte accesses.

---
 src/cmds.c   | 2 +-
 src/syntax.c | 4 +---
 src/xdisp.c  | 6 +++---
 3 files changed, 5 insertions(+), 7 deletions(-)

diff --git a/src/cmds.c b/src/cmds.c
index 9914b7a01f..2dd68ee067 100644
--- a/src/cmds.c
+++ b/src/cmds.c
@@ -399,7 +399,7 @@ internal_self_insert (int c, EMACS_INT n)
 		     by spaces so that the remaining text won't move.  */
 		  ptrdiff_t actual = PT_BYTE;
 		  DEC_POS (actual);
-		  if (FETCH_CHAR (actual) == '\t')
+		  if (FETCH_BYTE (actual) == '\t')
 		    /* Rather than add spaces, let's just keep the tab. */
 		    chars_to_delete--;
 		  else
diff --git a/src/syntax.c b/src/syntax.c
index a79ab86336..ef66f3ddd6 100644
--- a/src/syntax.c
+++ b/src/syntax.c
@@ -733,7 +733,6 @@ back_comment (ptrdiff_t from, ptrdiff_t from_byte, ptrdiff_t stop,
     {
       rarely_quit (++quit_count);
 
-      ptrdiff_t temp_byte;
       int prev_syntax;
       bool com2start, com2end, comstart;
 
@@ -884,8 +883,7 @@ back_comment (ptrdiff_t from, ptrdiff_t from_byte, ptrdiff_t stop,
 	  if (open_paren_in_column_0_is_defun_start
               && NILP (Vcomment_use_syntax_ppss)
 	      && (from == stop
-		  || (temp_byte = dec_bytepos (from_byte),
-		      FETCH_CHAR (temp_byte) == '\n')))
+		  || (FETCH_BYTE (from_byte - 1) == '\n')))
 	    {
 	      defun_start = from;
 	      defun_start_byte = from_byte;
diff --git a/src/xdisp.c b/src/xdisp.c
index bff6218106..4490878ccd 100644
--- a/src/xdisp.c
+++ b/src/xdisp.c
@@ -10474,7 +10474,7 @@ height (excluding the height of the mode- or header-line, if any) that
       while (bpos > BEGV_BYTE)
 	{
 	  DEC_BOTH (start, bpos);
-	  c = FETCH_CHAR (bpos);
+	  c = FETCH_BYTE (bpos);
 	  if (!(c == ' ' || c == '\t'))
 	    break;
 	}
@@ -10497,7 +10497,7 @@ height (excluding the height of the mode- or header-line, if any) that
       while (bpos > BEGV_BYTE)
 	{
 	  DEC_BOTH (end, bpos);
-	  c = FETCH_CHAR (bpos);
+	  c = FETCH_BYTE (bpos);
 	  if (!(c == ' ' || c == '\t' || c == '\n' || c == '\r'))
 	    break;
 	}
@@ -22119,7 +22119,7 @@ trailing_whitespace_p (ptrdiff_t charpos)
   int c = 0;
 
   while (bytepos < ZV_BYTE
-	 && (c = FETCH_CHAR (bytepos),
+	 && (c = FETCH_BYTE (bytepos),
 	     c == ' ' || c == '\t'))
     ++bytepos;
 
-- 
2.27.0.rc0