bug#36431: Crash in marker.c:337

bug#36431: Crash in marker.c:337

[Werner LEMBERG]

[-- Attachment #1: Type: Text/Plain, Size: 4587 bytes --]


Dear Emacs team,


while sending an e-mail with `mew', I get the attached crash.  This is
something new, which I haven't experienced before.  I can reliably
repeat it.


    Werner


======================================================================


In GNU Emacs 27.0.50 (build 2, x86_64-pc-linux-gnu, GTK+ Version 3.22.30, cairo version 1.15.10)
 of 2019-06-29 built on linux
Repository revision: 67b50770c050c55a26cd13b9568b01a80a449885
Repository branch: master
Windowing system distributor 'The X.Org Foundation', version 11.0.12003000
System Description: openSUSE Leap 15.1

Recent messages:
Quit
Setting up Mew world...
Updating status...done
Setting up Mew world...done
Scanning +inbox...done
Quit
Type C-x 1 to remove help window.  
Mark saved where search started
Quit
Making completion list...

Configured using:
 'configure --with-x-toolkit=gtk --with-cairo
 --enable-checking=yes,glyphs --enable-check-lisp-object-type
 'CC=ccache gcc' 'CFLAGS=-O0 -g3 -gdwarf-4''

Configured features:
XPM JPEG TIFF GIF PNG CAIRO SOUND DBUS GSETTINGS GLIB NOTIFY INOTIFY
GNUTLS LIBXML2 FREETYPE HARFBUZZ M17N_FLT LIBOTF ZLIB
TOOLKIT_SCROLL_BARS GTK3 X11 XDBE XIM THREADS PDUMPER GMP

Important settings:
  value of $LANG: en_US.UTF-8
  value of $XMODIFIERS: @im=none
  locale-coding-system: utf-8-unix

Major mode: Summary

Minor modes in effect:
  TeX-PDF-mode: t
  tooltip-mode: t
  global-eldoc-mode: t
  electric-indent-mode: t
  mouse-wheel-mode: t
  menu-bar-mode: t
  file-name-shadow-mode: t
  global-font-lock-mode: t
  font-lock-mode: t
  blink-cursor-mode: t
  auto-composition-mode: t
  auto-encryption-mode: t
  auto-compression-mode: t
  buffer-read-only: t
  column-number-mode: t
  transient-mark-mode: t

Load-path shadows:
/usr/local/share/emacs/site-lisp/thai-word hides /usr/local/share/emacs/27.0.50/lisp/language/thai-word

Features:
(shadow emacsbug message rmc puny dired dired-loaddefs format-spec
rfc822 mml mml-sec epa derived epg gnus-util rmail rmail-loaddefs
text-property-search time-date mm-decode mm-bodies mm-encode
mail-parse rfc2231 mailabbrev gmm-utils mailheader sendmail rfc2047
rfc2045 ietf-drums mail-utils misearch multi-isearch apropos pp
mew-varsx mew-unix elec-pair edmacro kmacro rng-nxml rng-valid rng-loc
rng-uri rng-parse nxml-parse rng-match rng-dt rng-util rng-pttrn
nxml-ns nxml-mode nxml-outln nxml-rap nxml-util nxml-enc xmltok
sgml-mode dom hideshow cal-menu calendar cal-loaddefs mew-auth
mew-config mew-imap2 mew-imap mew-nntp2 mew-nntp mew-pop mew-smtp
mew-ssl mew-ssh mew-net mew-highlight mew-sort mew-fib mew-ext
mew-refile mew-demo mew-attach mew-draft mew-message mew-thread
mew-virtual mew-summary4 mew-summary3 mew-summary2 mew-summary
mew-search mew-pick mew-passwd mew-scan mew-syntax mew-bq mew-smime
mew-pgp mew-header mew-exec mew-mark mew-mime mew-edit mew-decode
mew-encode mew-cache mew-minibuf mew-complete mew-addrbook mew-local
mew-vars3 mew-vars2 mew-vars mew-env mew-mule3 mew-mule mew-gemacs
mew-key mew-func mew-blvs mew-const mew tex dbus xml crm advice
auto-loads tex-site quail help-mode cjktilde mm-util mail-prsvr
disp-table finder-inf mule-util package easymenu epg-config
url-handlers url-parse auth-source cl-seq eieio eieio-core cl-macs
eieio-loaddefs password-cache json subr-x map url-vars seq byte-opt gv
bytecomp byte-compile cconv cl-loaddefs cl-lib tooltip eldoc electric
uniquify ediff-hook vc-hooks lisp-float-type mwheel term/x-win x-win
term/common-win x-dnd tool-bar dnd fontset image regexp-opt fringe
tabulated-list replace newcomment text-mode elisp-mode lisp-mode
prog-mode register page menu-bar rfn-eshadow isearch timer select
scroll-bar mouse jit-lock font-lock syntax facemenu font-core
term/tty-colors frame cl-generic cham georgian utf-8-lang misc-lang
vietnamese tibetan thai tai-viet lao korean japanese eucjp-ms cp51932
hebrew greek romanian slovak czech european ethiopic indian cyrillic
chinese composite charscript charprop case-table epa-hook
jka-cmpr-hook help simple abbrev obarray minibuffer cl-preloaded
nadvice loaddefs button faces cus-face macroexp files text-properties
overlay sha1 md5 base64 format env code-pages mule custom widget
hashtable-print-readable backquote threads dbusbind inotify
dynamic-setting system-font-setting font-render-setting cairo
move-toolbar gtk x-toolkit x multi-tty make-network-process emacs)

Memory information:
((conses 16 155296 8644)
 (symbols 48 16475 1)
 (strings 32 59212 1207)
 (string-bytes 1 1586649)
 (vectors 16 23308)
 (vector-slots 8 379039 13042)
 (floats 8 56 42)
 (intervals 56 1877 161)
 (buffers 992 14))



[-- Attachment #2: gdb.txt.xz --]
[-- Type: Application/Octet-Stream, Size: 6688 bytes --]

bug#36431: Crash in marker.c:337

[Eli Zaretskii]

> Date: Sat, 29 Jun 2019 13:17:34 +0200 (CEST)
> From: Werner LEMBERG <wl@gnu.org>
> 
> while sending an e-mail with `mew', I get the attached crash.  This is
> something new, which I haven't experienced before.  I can reliably
> repeat it.

Please send the reproduction recipe, preferably starting from "emacs
-Q", including any files required for the reproduction.  I don't think
it's possible to debug this otherwise, as the problem is clearly
data-dependent (somehow we end up requesting a byte-to-character
position conversion for a byte position that is not on character
boundary).

Thanks.

bug#36431: Crash in marker.c:337

[Eli Zaretskii]

> Date: Sat, 29 Jun 2019 15:13:26 +0300
> From: Eli Zaretskii <eliz@gnu.org>
> Cc: 36431@debbugs.gnu.org
> 
> > Date: Sat, 29 Jun 2019 13:17:34 +0200 (CEST)
> > From: Werner LEMBERG <wl@gnu.org>
> > 
> > while sending an e-mail with `mew', I get the attached crash.  This is
> > something new, which I haven't experienced before.  I can reliably
> > repeat it.
> 
> Please send the reproduction recipe, preferably starting from "emacs
> -Q", including any files required for the reproduction.  I don't think
> it's possible to debug this otherwise, as the problem is clearly
> data-dependent (somehow we end up requesting a byte-to-character
> position conversion for a byte position that is not on character
> boundary).

CC'ing Stefan, as it turns out he added that assertion not long ago.

bug#36431: Crash in marker.c:337

[Stefan Monnier]

>> > while sending an e-mail with `mew', I get the attached crash.  This is
>> > something new, which I haven't experienced before.  I can reliably
>> > repeat it.

Hmm... indeed insert-file-contents takes some shortcuts when inserting
the new text into the buffer which can break the expected invariants.

I don't really know how to reproduce your bug, but I think I have an
idea of what might be going on.
Can you try the patch below, to see if it fixes your problem?


        Stefan


diff --git a/src/fileio.c b/src/fileio.c
index ed1d2aedf3..a7be05ef5c 100644
--- a/src/fileio.c
+++ b/src/fileio.c
@@ -3741,6 +3741,7 @@ because (1) it preserves some marker positions and (2) it puts less data
 	  CHECK_CODING_SYSTEM (Vcoding_system_for_read);
 	  Fset (Qbuffer_file_coding_system, Vcoding_system_for_read);
 	}
+      eassert (inserted == 0);
       goto notfound;
     }
 
@@ -3767,7 +3768,10 @@ because (1) it preserves some marker positions and (2) it puts less data
       not_regular = 1;
 
       if (! NILP (visit))
-	goto notfound;
+        {
+          eassert (inserted == 0);
+	  goto notfound;
+        }
 
       if (! NILP (replace) || ! NILP (beg) || ! NILP (end))
 	xsignal2 (Qfile_error,
@@ -4435,19 +4439,6 @@ because (1) it preserves some marker positions and (2) it puts less data
   if (how_much < 0)
     report_file_error ("Read error", orig_filename);
 
-  /* Make the text read part of the buffer.  */
-  GAP_SIZE -= inserted;
-  GPT      += inserted;
-  GPT_BYTE += inserted;
-  ZV       += inserted;
-  ZV_BYTE  += inserted;
-  Z        += inserted;
-  Z_BYTE   += inserted;
-
-  if (GAP_SIZE > 0)
-    /* Put an anchor to ensure multi-byte form ends at gap.  */
-    *GPT_ADDR = 0;
-
  notfound:
 
   if (NILP (coding_system))
@@ -4457,6 +4448,7 @@ because (1) it preserves some marker positions and (2) it puts less data
 
 	 Note that we can get here only if the buffer was empty
 	 before the insertion.  */
+      eassert (Z == BEG);
 
       if (!NILP (Vcoding_system_for_read))
 	coding_system = Vcoding_system_for_read;
@@ -4477,6 +4469,16 @@ because (1) it preserves some marker positions and (2) it puts less data
 	  bset_undo_list (current_buffer, Qt);
 	  record_unwind_protect (decide_coding_unwind, unwind_data);
 
+          /* Make the text read part of the buffer.  */
+          eassert (NILP (BVAR (current_buffer, enable_multibyte_characters)));
+          GAP_SIZE -= inserted;
+          GPT      += inserted;
+          GPT_BYTE += inserted;
+          ZV       += inserted;
+          ZV_BYTE  += inserted;
+          Z        += inserted;
+          Z_BYTE   += inserted;
+
 	  if (inserted > 0 && ! NILP (Vset_auto_coding_function))
 	    {
 	      coding_system = call2 (Vset_auto_coding_function,
@@ -4493,8 +4495,18 @@ because (1) it preserves some marker positions and (2) it puts less data
 	      if (CONSP (coding_system))
 		coding_system = XCAR (coding_system);
 	    }
-	  unbind_to (count1, Qnil);
+
+          /* Move the text back into the gap.  Do it now, before we set the
+             buffer back to multibyte, since the bytes may very well not be
+             valid for a multibyte buffer.  */
+          set_point_both (BEG, BEG_BYTE);
+          move_gap_both (BEG, BEG_BYTE);
 	  inserted = Z_BYTE - BEG_BYTE;
+          GAP_SIZE += inserted;
+          ZV = Z = BEG;
+          ZV_BYTE = Z_BYTE = BEG_BYTE;
+
+	  unbind_to (count1, Qnil);
 	}
 
       if (NILP (coding_system))
@@ -4528,22 +4540,28 @@ because (1) it preserves some marker positions and (2) it puts less data
 	}
     }
 
+  eassert (PT == GPT);
+
   coding.dst_multibyte = ! NILP (BVAR (current_buffer, enable_multibyte_characters));
   if (CODING_MAY_REQUIRE_DECODING (&coding)
       && (inserted > 0 || CODING_REQUIRE_FLUSHING (&coding)))
     {
-      move_gap_both (PT, PT_BYTE);
-      GAP_SIZE += inserted;
-      ZV_BYTE -= inserted;
-      Z_BYTE -= inserted;
-      ZV -= inserted;
-      Z -= inserted;
       decode_coding_gap (&coding, inserted, inserted);
       inserted = coding.produced_char;
       coding_system = CODING_ID_NAME (coding.id);
     }
   else if (inserted > 0)
     {
+      /* Make the text read part of the buffer.  */
+      eassert (NILP (BVAR (current_buffer, enable_multibyte_characters)));
+      GAP_SIZE -= inserted;
+      GPT      += inserted;
+      GPT_BYTE += inserted;
+      ZV       += inserted;
+      ZV_BYTE  += inserted;
+      Z        += inserted;
+      Z_BYTE   += inserted;
+
       invalidate_buffer_caches (current_buffer, PT, PT + inserted);
       adjust_after_insert (PT, PT_BYTE, PT + inserted, PT_BYTE + inserted,
 			   inserted);

bug#36431: Crash in marker.c:337

[Werner LEMBERG]

>>> > while sending an e-mail with `mew', I get the attached crash.
>>> > This is something new, which I haven't experienced before.  I
>>> > can reliably repeat it.
> 
> Hmm... indeed insert-file-contents takes some shortcuts when
> inserting the new text into the buffer which can break the expected
> invariants.
> 
> I don't really know how to reproduce your bug, but I think I have an
> idea of what might be going on.  Can you try the patch below, to see
> if it fixes your problem?

No, it doesn't fix the problem, unfortunately – mew doesn't start at
all with this patch; the error message shows a string that is
definitely cropped.

My problem is that I have no idea how to reproduce the problem without
mew.  What approach do you suggest?


    Werner

bug#36431: Crash in marker.c:337

[Stefan Monnier]

> No, it doesn't fix the problem, unfortunately – mew doesn't start at
> all with this patch; the error message shows a string that is
> definitely cropped.

Yes, I also saw other problems with the patch.
I'll send an updated patch later.


        Stefan

bug#36431: Crash in marker.c:337

[Eli Zaretskii]

> From: Stefan Monnier <monnier@iro.umontreal.ca>
> Cc: wl@gnu.org,  36431@debbugs.gnu.org
> Date: Sat, 29 Jun 2019 18:56:53 -0400
> 
> I don't really know how to reproduce your bug, but I think I have an
> idea of what might be going on.
> Can you try the patch below, to see if it fixes your problem?

AFAICT, this patch moves the call to move_gap_both from a fragment
where we must decode the inserted text to a fragment where such a
decoding might not be necessary.  If I'm right, then this makes
insert-file-contents slower in some cases, because moving the gap
might be very expensive with large buffers.

More generally, I'd be leery to make significant changes ion
insert-file-contents just to placate that single assertion.  What do
we gain with that assertion except some theoretical correctness?
OTOH, the losses, in stability, performance, and not least our time
and energy is (or at least might be) real and tangible.  So why
bother?

bug#36431: Crash in marker.c:337

[Eli Zaretskii]

> Date: Sun, 30 Jun 2019 09:26:26 +0200 (CEST)
> Cc: eliz@gnu.org, 36431@debbugs.gnu.org
> From: Werner LEMBERG <wl@gnu.org>
> 
> My problem is that I have no idea how to reproduce the problem without
> mew.  What approach do you suggest?

From the backtrace, I see that the chain of calls is this:

  "insert-file-contents" (0xffff8ff8)
  "apply" (0xffff8ff0)
  "mew-insert-file-contents2" (0xffff95e8)
  "apply" (0xffff95e0)
  "mew-insert-file-contents" (0xffff9a40)
  "mew-encode-mime-body" (0xffffa250)
  "mew-encode-singlepart" (0xffffa830)
  "mew-encode-multipart" (0xffffadb0)
  "mew-encode-make-multi" (0xffffb2f0)
  "mew-smtp-encode-message" (0xffffb870)
  "mew-smtp-encode" (0xffffbd40)
  "mew-draft-smtp-process-message" (0xffffc2a0)
  "mew-draft-process-message" (0xffffc790)
  "mew-draft-send-message" (0xffffce80)

So my suggestion would be to try to find out what should be the buffer
contents before the call to insert-file-contents, and what should be
in the file being inserted, to reproduce the problem, and then post
the data needed for the reproduction.

If you cannot figure this out on the level of insert-file-contents,
try going up the call stack, and find the lowest level where you can
figure out the arguments and their values, including the contents of
any files needed for the reproduction.  Then show the results, and we
will try to use the relevant mew code to fill in the blanks.

One caveat: your mew customizations might be part of the puzzle, so be
sure to spell them out.

Thanks.

bug#36431: Crash in marker.c:337

[Stefan Monnier]

> AFAICT, this patch moves the call to move_gap_both from a fragment
> where we must decode the inserted text to a fragment where such a
> decoding might not be necessary.  If I'm right, then this makes
> insert-file-contents slower in some cases, because moving the gap
> might be very expensive with large buffers.

Indeed, that's one of the problems, but there are more serious
(correctness) problems with it anyway.

> More generally, I'd be leery to make significant changes ion
> insert-file-contents just to placate that single assertion.

I'm still trying to really understand what the code is doing, but so far
I get the impression that there are real bugs there, so I'm not really
concerned with resolving the assertion but with fixing those bugs.
Of course, to do that, I first need to understand the code (which maybe
will convince me that there aren't any bugs after all).


        Stefan

bug#36431: Crash in marker.c:337

[Eli Zaretskii]

> From: Stefan Monnier <monnier@iro.umontreal.ca>
> Cc: wl@gnu.org,  36431@debbugs.gnu.org
> Date: Sun, 30 Jun 2019 10:59:09 -0400
> 
> > More generally, I'd be leery to make significant changes ion
> > insert-file-contents just to placate that single assertion.
> 
> I'm still trying to really understand what the code is doing, but so far
> I get the impression that there are real bugs there, so I'm not really
> concerned with resolving the assertion but with fixing those bugs.
> Of course, to do that, I first need to understand the code (which maybe
> will convince me that there aren't any bugs after all).

I'm not sure those are bugs.  When we manipulate the gap, the buffer
is left in inconsistent state for short periods of time, and that is
normal AFAIU.

bug#36431: Crash in marker.c:337

[Stefan Monnier]

> I'm not sure those are bugs.

Neither am I.

> When we manipulate the gap, the buffer is left in inconsistent state
> for short periods of time, and that is normal AFAIU.

Yes, it's OK as long as it doesn't "escape".  That's what I'm not quite
sure of yet.


        Stefan

bug#36431: Crash in marker.c:337

[Stefan Monnier]

Stefan Monnier <monnier@iro.umontreal.ca> writes:

>> No, it doesn't fix the problem, unfortunately – mew doesn't start at
>> all with this patch; the error message shows a string that is
>> definitely cropped.
>
> Yes, I also saw other problems with the patch.
> I'll send an updated patch later.

Can you try the patch below?


        Stefan
        

diff --git a/src/fileio.c b/src/fileio.c
index ed1d2aedf3..1fea93fa8e 100644
--- a/src/fileio.c
+++ b/src/fileio.c
@@ -3741,6 +3741,7 @@ by calling `format-decode', which see.  */)
 	  CHECK_CODING_SYSTEM (Vcoding_system_for_read);
 	  Fset (Qbuffer_file_coding_system, Vcoding_system_for_read);
 	}
+      eassert (inserted == 0);
       goto notfound;
     }
 
@@ -3767,7 +3768,10 @@ by calling `format-decode', which see.  */)
       not_regular = 1;
 
       if (! NILP (visit))
-	goto notfound;
+        {
+          eassert (inserted == 0);
+	  goto notfound;
+        }
 
       if (! NILP (replace) || ! NILP (beg) || ! NILP (end))
 	xsignal2 (Qfile_error,
@@ -4435,19 +4439,6 @@ by calling `format-decode', which see.  */)
   if (how_much < 0)
     report_file_error ("Read error", orig_filename);
 
-  /* Make the text read part of the buffer.  */
-  GAP_SIZE -= inserted;
-  GPT      += inserted;
-  GPT_BYTE += inserted;
-  ZV       += inserted;
-  ZV_BYTE  += inserted;
-  Z        += inserted;
-  Z_BYTE   += inserted;
-
-  if (GAP_SIZE > 0)
-    /* Put an anchor to ensure multi-byte form ends at gap.  */
-    *GPT_ADDR = 0;
-
  notfound:
 
   if (NILP (coding_system))
@@ -4457,6 +4448,7 @@ by calling `format-decode', which see.  */)
 
 	 Note that we can get here only if the buffer was empty
 	 before the insertion.  */
+      eassert (Z == BEG);
 
       if (!NILP (Vcoding_system_for_read))
 	coding_system = Vcoding_system_for_read;
@@ -4477,6 +4469,10 @@ by calling `format-decode', which see.  */)
 	  bset_undo_list (current_buffer, Qt);
 	  record_unwind_protect (decide_coding_unwind, unwind_data);
 
+          /* Make the text read part of the buffer.  */
+          eassert (NILP (BVAR (current_buffer, enable_multibyte_characters)));
+          insert_from_gap_1 (inserted, inserted, false);
+
 	  if (inserted > 0 && ! NILP (Vset_auto_coding_function))
 	    {
 	      coding_system = call2 (Vset_auto_coding_function,
@@ -4493,8 +4489,22 @@ by calling `format-decode', which see.  */)
 	      if (CONSP (coding_system))
 		coding_system = XCAR (coding_system);
 	    }
-	  unbind_to (count1, Qnil);
+
+          /* Move the text back to the beginning of the gap.
+             Do it now, before we set the buffer back to multibyte, since the
+             bytes may very well not be valid for a multibyte buffer.  */
+          set_point_both (BEG, BEG_BYTE);
+          /* In general this may have to move all the bytes, but here
+             this can't move more bytes than were moved during the execution
+             of Vset_auto_coding_function, which is normally 0 (because it
+             normally doesn't modify the buffer).  */
+          move_gap_both (Z, Z_BYTE);
 	  inserted = Z_BYTE - BEG_BYTE;
+          GAP_SIZE += inserted;
+          ZV = Z = GPT = BEG;
+          ZV_BYTE = Z_BYTE = GPT_BYTE = BEG_BYTE;
+
+	  unbind_to (count1, Qnil);
 	}
 
       if (NILP (coding_system))
@@ -4528,22 +4538,29 @@ by calling `format-decode', which see.  */)
 	}
     }
 
-  coding.dst_multibyte = ! NILP (BVAR (current_buffer, enable_multibyte_characters));
+  eassert (PT == GPT);
+
+  coding.dst_multibyte
+    = ! NILP (BVAR (current_buffer, enable_multibyte_characters));
   if (CODING_MAY_REQUIRE_DECODING (&coding)
       && (inserted > 0 || CODING_REQUIRE_FLUSHING (&coding)))
     {
-      move_gap_both (PT, PT_BYTE);
-      GAP_SIZE += inserted;
-      ZV_BYTE -= inserted;
-      Z_BYTE -= inserted;
-      ZV -= inserted;
-      Z -= inserted;
+      /* Now we have all the new bytes at the beginning of the gap,
+         but `decode_coding_gap` needs them at the end of the gap, so
+         we need to move them.
+         FIXME: We should arrange for the bytes to be already at the right
+         place so we don't need to memmove them in the common case!  */
+      memmove (GAP_END_ADDR - inserted, GPT_ADDR, inserted);
       decode_coding_gap (&coding, inserted, inserted);
       inserted = coding.produced_char;
       coding_system = CODING_ID_NAME (coding.id);
     }
   else if (inserted > 0)
     {
+      /* Make the text read part of the buffer.  */
+      eassert (NILP (BVAR (current_buffer, enable_multibyte_characters)));
+      insert_from_gap_1 (inserted, inserted, false);
+
       invalidate_buffer_caches (current_buffer, PT, PT + inserted);
       adjust_after_insert (PT, PT_BYTE, PT + inserted, PT_BYTE + inserted,
 			   inserted);
diff --git a/src/insdel.c b/src/insdel.c
index 85fffd8fd1..51371ee13c 100644
--- a/src/insdel.c
+++ b/src/insdel.c
@@ -115,7 +115,7 @@ gap_left (ptrdiff_t charpos, ptrdiff_t bytepos, bool newgap)
   i = GPT_BYTE;
   to = GAP_END_ADDR;
   from = GPT_ADDR;
-  new_s1 = GPT_BYTE;
+  new_s1 = GPT_BYTE; /* May point in the middle of multibyte sequences.  */
 
   /* Now copy the characters.  To move the gap down,
      copy characters up.  */
@@ -133,6 +133,7 @@ gap_left (ptrdiff_t charpos, ptrdiff_t bytepos, bool newgap)
 	 make_gap_smaller set inhibit-quit.  */
       if (QUITP)
 	{
+          /* FIXME: This can point in the middle of a multibyte character.  */
 	  bytepos = new_s1;
 	  charpos = BYTE_TO_CHAR (bytepos);
 	  break;
@@ -164,7 +165,7 @@ gap_right (ptrdiff_t charpos, ptrdiff_t bytepos)
 {
   register unsigned char *to, *from;
   register ptrdiff_t i;
-  ptrdiff_t new_s1;
+  ptrdiff_t new_s1; /* May point in the middle of multibyte sequences.  */
 
   BUF_COMPUTE_UNCHANGED (current_buffer, charpos, GPT);
 
@@ -189,6 +190,7 @@ gap_right (ptrdiff_t charpos, ptrdiff_t bytepos)
 	 make_gap_smaller set inhibit-quit.  */
       if (QUITP)
 	{
+          /* FIXME: This can point in the middle of a multibyte character.  */
 	  bytepos = new_s1;
 	  charpos = BYTE_TO_CHAR (bytepos);
 	  break;
@@ -1072,6 +1074,31 @@ insert_from_string_1 (Lisp_Object string, ptrdiff_t pos, ptrdiff_t pos_byte,
 \f
 /* Insert a sequence of NCHARS chars which occupy NBYTES bytes
    starting at GAP_END_ADDR - NBYTES (if text_at_gap_tail) and at
+   GPT_ADDR (if not text_at_gap_tail).
+   Contrary to insert_from_gap, this does not invalidate any cache,
+   nor update any markers, nor record any buffer modification information
+   of any sort.  */
+void
+insert_from_gap_1 (ptrdiff_t nchars, ptrdiff_t nbytes, bool text_at_gap_tail)
+{
+  GAP_SIZE -= nbytes;
+  if (! text_at_gap_tail)
+    {
+      GPT += nchars;
+      GPT_BYTE += nbytes;
+    }
+  ZV += nchars;
+  Z += nchars;
+  ZV_BYTE += nbytes;
+  Z_BYTE += nbytes;
+
+  /* Put an anchor to ensure multi-byte form ends at gap.  */
+  if (GAP_SIZE > 0) *(GPT_ADDR) = 0;
+  eassert (GPT <= GPT_BYTE);
+}
+
+/* Insert a sequence of NCHARS chars which occupy NBYTES bytes
+   starting at GAP_END_ADDR - NBYTES (if text_at_gap_tail) and at
    GPT_ADDR (if not text_at_gap_tail).  */
 
 void
@@ -1090,19 +1117,7 @@ insert_from_gap (ptrdiff_t nchars, ptrdiff_t nbytes, bool text_at_gap_tail)
   record_insert (GPT, nchars);
   modiff_incr (&MODIFF);
 
-  GAP_SIZE -= nbytes;
-  if (! text_at_gap_tail)
-    {
-      GPT += nchars;
-      GPT_BYTE += nbytes;
-    }
-  ZV += nchars;
-  Z += nchars;
-  ZV_BYTE += nbytes;
-  Z_BYTE += nbytes;
-  if (GAP_SIZE > 0) *(GPT_ADDR) = 0; /* Put an anchor.  */
-
-  eassert (GPT <= GPT_BYTE);
+  insert_from_gap_1 (nchars, nbytes, text_at_gap_tail);
 
   adjust_overlays_for_insert (ins_charpos, nchars);
   adjust_markers_for_insert (ins_charpos, ins_bytepos,
diff --git a/src/lisp.h b/src/lisp.h
index a0619e64f2..1a1d8ee7e4 100644
--- a/src/lisp.h
+++ b/src/lisp.h
@@ -3667,6 +3667,7 @@ extern void insert (const char *, ptrdiff_t);
 extern void insert_and_inherit (const char *, ptrdiff_t);
 extern void insert_1_both (const char *, ptrdiff_t, ptrdiff_t,
 			   bool, bool, bool);
+extern void insert_from_gap_1 (ptrdiff_t, ptrdiff_t, bool text_at_gap_tail);
 extern void insert_from_gap (ptrdiff_t, ptrdiff_t, bool text_at_gap_tail);
 extern void insert_from_string (Lisp_Object, ptrdiff_t, ptrdiff_t,
 				ptrdiff_t, ptrdiff_t, bool);

bug#36431: Crash in marker.c:337

[Stefan Monnier]

>> I don't really know how to reproduce your bug, but I think I have an
>> idea of what might be going on.
>> Can you try the patch below, to see if it fixes your problem?
> AFAICT, this patch moves the call to move_gap_both from a fragment
> where we must decode the inserted text to a fragment where such a
> decoding might not be necessary.  If I'm right, then this makes
> insert-file-contents slower in some cases, because moving the gap
> might be very expensive with large buffers.

Indeed.  It also removed the move_gap_both from the case where we need
to decode and we already know the coding-system to use.  So in some
cases it made it faster (these are the cases where it misbehaved).
The new version of the code shouldn't suffer from this performance
problem (it still calls move_gap_both in the set-auto-coding part of
the code, but this call should have a cost proportional to the amount
of buffer modification performed by set-auto-coding, i.e. it should be
a nop in pretty much all cases).

Looking at this aspect (i.e. not directly related to this bug) I'm
wondering why the code works this way:

We start by inserting the new bytes at the *beginning* of the gap, but
when we do the move_gap_both this moves those bytes to the *end* of the
gap (where decode_coding_gap expects them, apparently), so when we
decode we always have to move all the inserted bytes, right?

> More generally, I'd be leery to make significant changes ion
> insert-file-contents just to placate that single assertion.  What do
> we gain with that assertion except some theoretical correctness?

Again, I'm just trying to understand the code at this point.
The part that worries me is the following:

In the current code, we read the raw bytes to the beginning of the gap,
then (when Vset_auto_coding_function needs to be called), we (virtually)
move them into the current buffer, which is usually multibyte.
AFAICT at this point we have a buffer in a transiently inconsistent
state since it's multibyte but it can contain arbitrary byte sequences,
hence invalid byte sequences.  Before calling Vset_auto_coding_function
we make this buffer unibyte, which brings us back to a consistent state,
but I wonder if/how/why making the buffer unibyte and then back to
multibyte always preserves the original byte sequence, since AFAICT
set-buffer-multibyte will always make the effort to bring the buffer to
a consistent state, so if the state is inconsistent before the pair of
calls to set-buffer-multibyte, either we changed the byte sequence or 
set-buffer-multibyte doesn't always result in a consistent state.
What am I missing?


        Stefan

bug#36431: Crash in marker.c:337

[Stefan Monnier]

> In the current code, we read the raw bytes to the beginning of the gap,
> then (when Vset_auto_coding_function needs to be called), we (virtually)
> move them into the current buffer, which is usually multibyte.
> AFAICT at this point we have a buffer in a transiently inconsistent
> state since it's multibyte but it can contain arbitrary byte sequences,
> hence invalid byte sequences.  Before calling Vset_auto_coding_function
> we make this buffer unibyte, which brings us back to a consistent state,
> but I wonder if/how/why making the buffer unibyte and then back to
> multibyte always preserves the original byte sequence, since AFAICT
> set-buffer-multibyte will always make the effort to bring the buffer to
> a consistent state, so if the state is inconsistent before the pair of
> calls to set-buffer-multibyte, either we changed the byte sequence or 
> set-buffer-multibyte doesn't always result in a consistent state.
> What am I missing?

OK, I finally saw that we don't actually call set-buffer-multibyte but
instead we just set bset_enable_multibyte_characters.  I'm beginning to
understand better.


        Stefan

bug#36431: Crash in marker.c:337

[Stefan Monnier]

> OK, I finally saw that we don't actually call set-buffer-multibyte but
> instead we just set bset_enable_multibyte_characters.  I'm beginning to
> understand better.

Alright, so here one way to make this "transient" inconsistent state
escape its transientness (should it be "transienticity"?) and crash Emacs:

    (catch 'toto
      (let ((set-auto-coding-function (lambda (rest _) (throw 'toto nil))))
        (erase-buffer)
        (insert-file-contents "~/tmp/foo-c3c3.txt")))
        
where `~/tmp/foo-c3c3.txt` is a file that contains some invalid
utf-8 byte sequence (in my case I used two bytes C3 together).


        Stefan

bug#36431: Crash in marker.c:337

[Eli Zaretskii]

> From: Stefan Monnier <monnier@iro.umontreal.ca>
> Cc: wl@gnu.org,  36431@debbugs.gnu.org
> Date: Tue, 02 Jul 2019 13:04:38 -0400
> 
> We start by inserting the new bytes at the *beginning* of the gap, but
> when we do the move_gap_both this moves those bytes to the *end* of the
> gap (where decode_coding_gap expects them, apparently), so when we
> decode we always have to move all the inserted bytes, right?

Yes.  This is so we don't need to know up front how many bytes will be
read from the file.

bug#36431: Crash in marker.c:337

[Eli Zaretskii]

> Alright, so here one way to make this "transient" inconsistent state
> escape its transientness (should it be "transienticity"?) and crash Emacs:
> 
>     (catch 'toto
>       (let ((set-auto-coding-function (lambda (rest _) (throw 'toto nil))))
>         (erase-buffer)
>         (insert-file-contents "~/tmp/foo-c3c3.txt")))
>         
> where `~/tmp/foo-c3c3.txt` is a file that contains some invalid
> utf-8 byte sequence (in my case I used two bytes C3 together).

You are saying that decide_coding_unwind should do a better job?

bug#36431: Crash in marker.c:337

[Stefan Monnier]

>> We start by inserting the new bytes at the *beginning* of the gap, but
>> when we do the move_gap_both this moves those bytes to the *end* of the
>> gap (where decode_coding_gap expects them, apparently), so when we
>> decode we always have to move all the inserted bytes, right?
>
> Yes.  This is so we don't need to know up front how many bytes will be
> read from the file.

OK, so IIUC:
- we insert the new bytes at the beginning of the gap, in order to have
  room to grow if there are more bytes than expected, and also in case
  there are fewer bytes than expected (in which case we'd otherwise
  have to move the bytes we just read so they properly end at the end
  of the gap).
- decode_coding_gap wants the new input bytes to be at the end of the
  gap, so that we can put the decoded chars at the beginning of the gap
  and as one grows the other shrinks, so we don't need space for "IN +
  OUT" bytes but only for "OUT" bytes.  Is that right (I'm trying to
  find some comment or other evidence that this is the case, but
  haven't found it yet).

IOW, it should be possible to optimize the common case by reading the
new bytes into the end of the gap to avoid moving everything in the
common case (if the number of bytes read is different from originally
expected, we'll have to do extra work, but for the common case where we
know the file size upfront and it doesn't change while we read it, this
will save us some work).

But the effort is probably not worth the trouble: a memmove of a few
gigabytes costs relatively little compared to the cost of actually
decoding those same gigabytes.


        Stefan

bug#36431: Crash in marker.c:337

[Stefan Monnier]

>> Alright, so here one way to make this "transient" inconsistent state
>> escape its transientness (should it be "transienticity"?) and crash Emacs:
>> 
>>     (catch 'toto
>>       (let ((set-auto-coding-function (lambda (rest _) (throw 'toto nil))))
>>         (erase-buffer)
>>         (insert-file-contents "~/tmp/foo-c3c3.txt")))
>>         
>> where `~/tmp/foo-c3c3.txt` is a file that contains some invalid
>> utf-8 byte sequence (in my case I used two bytes C3 together).
> You are saying that decide_coding_unwind should do a better job?

Not sure yet.  In any case, it's a rather contrived situation.


        Stefan

bug#36431: Crash in marker.c:337

[Eli Zaretskii]

> From: Stefan Monnier <monnier@iro.umontreal.ca>
> Cc: wl@gnu.org,  36431@debbugs.gnu.org
> Date: Tue, 02 Jul 2019 13:51:30 -0400
> 
> - we insert the new bytes at the beginning of the gap, in order to have
>   room to grow if there are more bytes than expected, and also in case
>   there are fewer bytes than expected (in which case we'd otherwise
>   have to move the bytes we just read so they properly end at the end
>   of the gap).

Also, you will see in insert-file-contents that it supports quitting
while reading a huge file, and also the REPLACE argument, where we
detect the same contents at beginning and end of the file and the
buffer.

> - decode_coding_gap wants the new input bytes to be at the end of the
>   gap, so that we can put the decoded chars at the beginning of the gap
>   and as one grows the other shrinks, so we don't need space for "IN +
>   OUT" bytes but only for "OUT" bytes.  Is that right (I'm trying to
>   find some comment or other evidence that this is the case, but
>   haven't found it yet).

That's right.  The comment you are looking for (well, at least part of
it) is in the commentary before decode_coding, where it explains the
semantics of CODING->src_pos.  You will see at the beginning of
decode_coding_gap how it sets things up according to that hairy
protocol.

> IOW, it should be possible to optimize the common case by reading the
> new bytes into the end of the gap to avoid moving everything in the
> common case (if the number of bytes read is different from originally
> expected, we'll have to do extra work, but for the common case where we
> know the file size upfront and it doesn't change while we read it, this
> will save us some work).
> 
> But the effort is probably not worth the trouble: a memmove of a few
> gigabytes costs relatively little compared to the cost of actually
> decoding those same gigabytes.

Right.  Also, there are the other subtle issues with quitting, the
REPLACE argument, special files, etc.

bug#36431: Crash in marker.c:337

[Stefan Monnier]

>> - we insert the new bytes at the beginning of the gap, in order to have
>>   room to grow if there are more bytes than expected, and also in case
>>   there are fewer bytes than expected (in which case we'd otherwise
>>   have to move the bytes we just read so they properly end at the end
>>   of the gap).
>
> Also, you will see in insert-file-contents that it supports quitting
> while reading a huge file, and also the REPLACE argument, where we
> detect the same contents at beginning and end of the file and the
> buffer.

Right, tho the end result is the same (e.g. when we quit, we can either
abort the whole operation and trow away the bytes we read, or we can
keep going with the bytes we did read which is simply another case of
reading less than expected).

>> - decode_coding_gap wants the new input bytes to be at the end of the
>>   gap, so that we can put the decoded chars at the beginning of the gap
>>   and as one grows the other shrinks, so we don't need space for "IN +
>>   OUT" bytes but only for "OUT" bytes.  Is that right (I'm trying to
>>   find some comment or other evidence that this is the case, but
>>   haven't found it yet).
>
> That's right.  The comment you are looking for (well, at least part of
> it) is in the commentary before decode_coding, where it explains the
> semantics of CODING->src_pos.  You will see at the beginning of
> decode_coding_gap how it sets things up according to that hairy
> protocol.

IIUC you're referring to this comment:

    Decode the data at CODING->src_object into CODING->dst_object.
    CODING->src_object is a buffer, a string, or nil.
    CODING->dst_object is a buffer.
 
    If CODING->src_object is a buffer, it must be the current buffer.
    In this case, if CODING->src_pos is positive, it is a position of
    the source text in the buffer, otherwise, the source text is in the
    gap area of the buffer, and CODING->src_pos specifies the offset of
    the text from GPT (which must be the same as PT).  If this is the
    same buffer as CODING->dst_object, CODING->src_pos must be
    negative.
 
    If CODING->src_object is a string, CODING->src_pos is an index to
    that string.
 
    If CODING->src_object is nil, CODING->source must already point to
    the non-relocatable memory area.  In this case, CODING->src_pos is
    an offset from CODING->source.
 
    The decoded data is inserted at the current point of the buffer
    CODING->dst_object.

but this doesn't say if the bytes are to be found originally at the
beginning of the gap or its end, nor whether they finish at the beginning or
the end, nor what happens in the middle and why it's been designed this way.

Is the patch below correct?

>> IOW, it should be possible to optimize the common case by reading the
>> new bytes into the end of the gap to avoid moving everything in the
>> common case (if the number of bytes read is different from originally
>> expected, we'll have to do extra work, but for the common case where we
>> know the file size upfront and it doesn't change while we read it, this
>> will save us some work).
>> 
>> But the effort is probably not worth the trouble: a memmove of a few
>> gigabytes costs relatively little compared to the cost of actually
>> decoding those same gigabytes.
>
> Right.  Also, there are the other subtle issues with quitting, the
> REPLACE argument, special files, etc.

I think the crash-example I sent can probably be made less esoteric by
making it use "quit" instead of catch/throw.  I'm beginning to think
that when we quit (or signal an error) from within
set-auto-coding-function, we simply shouldn't revert the buffer
to multibyte.


        Stefan


diff --git a/src/coding.c b/src/coding.c
index 5b9bfa17dd..218d69e2e7 100644
--- a/src/coding.c
+++ b/src/coding.c
@@ -7322,11 +7322,16 @@ produce_annotation (struct coding_system *coding, ptrdiff_t pos)
 
    If CODING->src_object is a buffer, it must be the current buffer.
    In this case, if CODING->src_pos is positive, it is a position of
-   the source text in the buffer, otherwise, the source text is in the
-   gap area of the buffer, and CODING->src_pos specifies the offset of
-   the text from GPT (which must be the same as PT).  If this is the
-   same buffer as CODING->dst_object, CODING->src_pos must be
-   negative.
+   the source text in the buffer, otherwise, the source text is at the
+   end of the gap area of the buffer, and CODING->src_pos specifies the
+   offset of the text from the end of the gap (which must be the at PT).
+   If this is the same buffer as CODING->dst_object, CODING->src_pos must
+   be negative.
+   
+   When the text is taken from the gap, it needs to be at the end of
+   the gap so that we can produce the decoded text at the beginning of
+   the gap: this way, as the output grows, the input shrinks, so we only
+   need to allocate enough space for `max(IN, OUT)` instead of `IN + OUT`.
 
    If CODING->src_object is a string, CODING->src_pos is an index to
    that string.

bug#36431: Crash in marker.c:337

[Eli Zaretskii]

> From: Stefan Monnier <monnier@iro.umontreal.ca>
> Cc: wl@gnu.org,  36431@debbugs.gnu.org
> Date: Tue, 02 Jul 2019 15:44:07 -0400
> 
>     Decode the data at CODING->src_object into CODING->dst_object.
>     CODING->src_object is a buffer, a string, or nil.
>     CODING->dst_object is a buffer.
>  
>     If CODING->src_object is a buffer, it must be the current buffer.
>     In this case, if CODING->src_pos is positive, it is a position of
>     the source text in the buffer, otherwise, the source text is in the
>     gap area of the buffer, and CODING->src_pos specifies the offset of
>     the text from GPT (which must be the same as PT).  If this is the
>     same buffer as CODING->dst_object, CODING->src_pos must be
>     negative.
>  [...]
>     The decoded data is inserted at the current point of the buffer
>     CODING->dst_object.
> 
> but this doesn't say if the bytes are to be found originally at the
> beginning of the gap or its end, nor whether they finish at the beginning or
> the end, nor what happens in the middle and why it's been designed this way.

It says that (a) CODING->src_pos is the negative of the offset from
GPT of where the bytes are in the gap (they don't have to be "at the
end", AFAIU, just not "at the beginning"); and (b) that the decoded
text is inserted at point.  And those are, AFAIK, the only real
conditions, all the rest is not necessary, it's just what's
convenient.

As for why this was designed like that -- where else did you see
comments in Emacs that answer this kind of questions?

> Is the patch below correct?

I think it describes conditions that don't need to exist.

> I think the crash-example I sent can probably be made less esoteric by
> making it use "quit" instead of catch/throw.  I'm beginning to think
> that when we quit (or signal an error) from within
> set-auto-coding-function, we simply shouldn't revert the buffer
> to multibyte.

We have code whose purpose is to recover from such calamities, so if
it doesn't do its job in all cases, we need to augment it.

bug#36431: Crash in marker.c:337

[Stefan Monnier]

>>     Decode the data at CODING->src_object into CODING->dst_object.
>>     CODING->src_object is a buffer, a string, or nil.
>>     CODING->dst_object is a buffer.
>>  
>>     If CODING->src_object is a buffer, it must be the current buffer.
>>     In this case, if CODING->src_pos is positive, it is a position of
>>     the source text in the buffer, otherwise, the source text is in the
>>     gap area of the buffer, and CODING->src_pos specifies the offset of
>>     the text from GPT (which must be the same as PT).  If this is the
>>     same buffer as CODING->dst_object, CODING->src_pos must be
>>     negative.
>>  [...]
>>     The decoded data is inserted at the current point of the buffer
>>     CODING->dst_object.
>> 
>> but this doesn't say if the bytes are to be found originally at the
>> beginning of the gap or its end, nor whether they finish at the beginning or
>> the end, nor what happens in the middle and why it's been designed this way.
>
> It says that (a) CODING->src_pos is the negative of the offset from
> GPT of where the bytes are in the gap

Yes, I think this is actually wrong.
E.e. decode_coding_gap does:

    coding->src_chars = chars;
    [...]
    coding->src_pos = -chars;

so nowhere does it account for unspecified number of bytes between the
beginning of the gap and the beginning of the source text.
Here, `src_pos` is the offset from the end of the gap.

> (they don't have to be "at the
> end", AFAIU, just not "at the beginning");

Oh, indeed, src_pos doesn't need to start as the negation of src_chars. 

> As for why this was designed like that -- where else did you see
> comments in Emacs that answer this kind of questions?

There are such design comments at various places.
Usually added after the fact, sometimes added as part of
a commit-reversal to make sure someone else doesn't fall into the same
trap ;-)

>> Is the patch below correct?
> I think it describes conditions that don't need to exist.

How 'bout this new version.


        Stefan


diff --git a/src/coding.c b/src/coding.c
index 59589caee6..fd7e7aca0f 100644
--- a/src/coding.c
+++ b/src/coding.c
@@ -7323,10 +7323,15 @@ produce_annotation (struct coding_system *coding, ptrdiff_t pos)
    If CODING->src_object is a buffer, it must be the current buffer.
    In this case, if CODING->src_pos is positive, it is a position of
    the source text in the buffer, otherwise, the source text is in the
-   gap area of the buffer, and CODING->src_pos specifies the offset of
-   the text from GPT (which must be the same as PT).  If this is the
-   same buffer as CODING->dst_object, CODING->src_pos must be
-   negative.
+   gap area of the buffer, and CODING->src_pos specifies the
+   offset of the text from the end of the gap (which must be at PT).
+   If this is the same buffer as CODING->dst_object, CODING->src_pos must
+   be negative.
+
+   When the text is taken from the gap, it can't be at the beginning of
+   the gap so that we can produce the decoded text at the beginning of
+   the gap: this way, as the output grows, the input shrinks, so we only
+   need to allocate enough space for `max(IN, OUT)` instead of `IN + OUT`.
 
    If CODING->src_object is a string, CODING->src_pos is an index to
    that string.

bug#36431: Crash in marker.c:337

[Stefan Monnier]

> AFAICT, this patch moves the call to move_gap_both from a fragment
> where we must decode the inserted text to a fragment where such a
> decoding might not be necessary.  If I'm right, then this makes
> insert-file-contents slower in some cases, because moving the gap
> might be very expensive with large buffers.

Here's an alternative patch which doesn't suffer from this problem but
also eliminates the transiently-inconsistent multibyte buffer situation.


        Stefan


diff --git a/src/fileio.c b/src/fileio.c
index 2825c1b54c..9ed1fcf8ca 100644
--- a/src/fileio.c
+++ b/src/fileio.c
@@ -3705,6 +3705,7 @@ because (1) it preserves some marker positions and (2) it puts less data
 	  CHECK_CODING_SYSTEM (Vcoding_system_for_read);
 	  Fset (Qbuffer_file_coding_system, Vcoding_system_for_read);
 	}
+      eassert (inserted == 0);
       goto notfound;
     }
 
@@ -3731,7 +3732,10 @@ because (1) it preserves some marker positions and (2) it puts less data
       not_regular = 1;
 
       if (! NILP (visit))
-	goto notfound;
+        {
+          eassert (inserted == 0);
+	  goto notfound;
+        }
 
       if (! NILP (replace) || ! NILP (beg) || ! NILP (end))
 	xsignal2 (Qfile_error,
@@ -4399,10 +4403,10 @@ because (1) it preserves some marker positions and (2) it puts less data
   if (how_much < 0)
     report_file_error ("Read error", orig_filename);
 
-  /* Make the text read part of the buffer.  */
-  insert_from_gap_1 (inserted, inserted, false);
-
- notfound:
+ notfound: ;
+  Lisp_Object multibyte
+    = BVAR (current_buffer, enable_multibyte_characters);
+  bool ingap = true; /* Bytes are currently in the gap.  */
 
   if (NILP (coding_system))
     {
@@ -4411,6 +4415,7 @@ because (1) it preserves some marker positions and (2) it puts less data
 
 	 Note that we can get here only if the buffer was empty
 	 before the insertion.  */
+      eassert (Z == BEG);
 
       if (!NILP (Vcoding_system_for_read))
 	coding_system = Vcoding_system_for_read;
@@ -4421,8 +4426,6 @@ because (1) it preserves some marker positions and (2) it puts less data
 	     enable-multibyte-characters directly here without taking
 	     care of marker adjustment.  By this way, we can run Lisp
 	     program safely before decoding the inserted text.  */
-          Lisp_Object multibyte
-            = BVAR (current_buffer, enable_multibyte_characters);
           Lisp_Object undo_list = BVAR (current_buffer, undo_list);
 	  ptrdiff_t count1 = SPECPDL_INDEX ();
 
@@ -4430,6 +4433,10 @@ because (1) it preserves some marker positions and (2) it puts less data
 	  bset_undo_list (current_buffer, Qt);
 	  record_unwind_protect (restore_buffer, Fcurrent_buffer ());
 
+          /* Make the text read part of the buffer.  */
+          insert_from_gap_1 (inserted, inserted, false);
+          ingap = false;
+
 	  if (inserted > 0 && ! NILP (Vset_auto_coding_function))
 	    {
 	      coding_system = call2 (Vset_auto_coding_function,
@@ -4455,15 +4462,10 @@ because (1) it preserves some marker positions and (2) it puts less data
           adjust_overlays_for_delete (BEG, Z - BEG);
           set_buffer_intervals (current_buffer, NULL);
           TEMP_SET_PT_BOTH (BEG, BEG_BYTE);
-
-          /* Change the buffer's multibyteness directly.  We used to do this
-             from within unbind_to, but it was unsafe since the bytes
-             may contain invalid sequences for a multibyte buffer (which is OK
-             here since we'll decode them before anyone else gets to see
-             them, but is dangerous when we're doing a non-local exit).  */
-          bset_enable_multibyte_characters (current_buffer, multibyte);
           bset_undo_list (current_buffer, undo_list);
           inserted = Z_BYTE - BEG_BYTE;
+          /* The bytes may be invalid for a multibyte buffer, so we can't
+             restore the multibyteness yet.  */
         }
 
       if (NILP (coding_system))
@@ -4471,7 +4473,7 @@ because (1) it preserves some marker positions and (2) it puts less data
       else
 	CHECK_CODING_SYSTEM (coding_system);
 
-      if (NILP (BVAR (current_buffer, enable_multibyte_characters)))
+      if (NILP (multibyte))
 	/* We must suppress all character code conversion except for
 	   end-of-line conversion.  */
 	coding_system = raw_text_coding_system (coding_system);
@@ -4490,33 +4492,51 @@ because (1) it preserves some marker positions and (2) it puts less data
 	{
 	  /* Visiting a file with these coding system makes the buffer
 	     unibyte.  */
-	  if (inserted > 0)
+          if (!ingap)
+            multibyte = Qnil;
+	  else if (inserted > 0)
 	    bset_enable_multibyte_characters (current_buffer, Qnil);
-	  else
+          else
 	    Fset_buffer_multibyte (Qnil);
 	}
     }
 
-  coding.dst_multibyte = ! NILP (BVAR (current_buffer, enable_multibyte_characters));
+  coding.dst_multibyte = !NILP (multibyte);
   if (CODING_MAY_REQUIRE_DECODING (&coding)
       && (inserted > 0 || CODING_REQUIRE_FLUSHING (&coding)))
     {
-      move_gap_both (PT, PT_BYTE);
-      GAP_SIZE += inserted;
-      ZV_BYTE -= inserted;
-      Z_BYTE -= inserted;
-      ZV -= inserted;
-      Z -= inserted;
+      if (ingap)
+        { /* Text is at beginning of gap, move it to the end.  */
+          memmove (GAP_END_ADDR - inserted, GPT_ADDR, inserted);
+        }
+      else
+        { /* Text is inside the buffer; move it to end of the gap.  */
+          move_gap_both (PT, PT_BYTE);
+	  eassert (inserted == Z_BYTE - BEG_BYTE);
+          GAP_SIZE += inserted;
+          ZV = Z = GPT = BEG;
+          ZV_BYTE = Z_BYTE = GPT_BYTE = BEG_BYTE;
+          /* Now we are safe to change the buffer's multibyteness directly.  */
+          bset_enable_multibyte_characters (current_buffer, multibyte);
+        }
+
       decode_coding_gap (&coding, inserted);
       inserted = coding.produced_char;
       coding_system = CODING_ID_NAME (coding.id);
     }
-  else if (inserted > 0)
+  else if (inserted > 0 && ingap)
     {
+      /* Make the text read part of the buffer.  */
+      eassert (NILP (BVAR (current_buffer, enable_multibyte_characters)));
+      insert_from_gap_1 (inserted, inserted, false);
       invalidate_buffer_caches (current_buffer, PT, PT + inserted);
       adjust_after_insert (PT, PT_BYTE, PT + inserted, PT_BYTE + inserted,
 			   inserted);
     }
+  else if (!ingap)
+    { /* Apparently, no decoding needed, so just set the bytenesss.  */
+      bset_enable_multibyte_characters (current_buffer, multibyte);
+    }
 
   /* Call after-change hooks for the inserted text, aside from the case
      of normal visiting (not with REPLACE), which is done in a new buffer

bug#36431: Crash in marker.c:337

[Eli Zaretskii]

> From: Stefan Monnier <monnier@iro.umontreal.ca>
> Cc: wl@gnu.org,  36431@debbugs.gnu.org
> Date: Tue, 02 Jul 2019 17:00:21 -0400
> 
> > It says that (a) CODING->src_pos is the negative of the offset from
> > GPT of where the bytes are in the gap
> 
> Yes, I think this is actually wrong.

You are right.

> E.e. decode_coding_gap does:
> 
>     coding->src_chars = chars;
>     [...]
>     coding->src_pos = -chars;
> 
> so nowhere does it account for unspecified number of bytes between the
> beginning of the gap and the beginning of the source text.
> Here, `src_pos` is the offset from the end of the gap.

Yes, the clear evidence is in coding_set_source:

      if (coding->src_pos < 0)
	coding->source = BUF_GAP_END_ADDR (buf) + coding->src_pos_byte;

(Note that it actually only uses the byte offset's numerical value.  I
couldn't find any place where it actually uses the character offset in
src_pos, it only checks its sign.)

> > As for why this was designed like that -- where else did you see
> > comments in Emacs that answer this kind of questions?
> 
> There are such design comments at various places.
> Usually added after the fact, sometimes added as part of
> a commit-reversal to make sure someone else doesn't fall into the same
> trap ;-)

Interesting.  Can you point me to a couple of such design comments?

>     If CODING->src_object is a buffer, it must be the current buffer.
>     In this case, if CODING->src_pos is positive, it is a position of
>     the source text in the buffer, otherwise, the source text is in the
> -   gap area of the buffer, and CODING->src_pos specifies the offset of
> -   the text from GPT (which must be the same as PT).  If this is the
> -   same buffer as CODING->dst_object, CODING->src_pos must be
> -   negative.
> +   gap area of the buffer, and CODING->src_pos specifies the
> +   offset of the text from the end of the gap (which must be at PT).

The "which must be at PT" part is ambiguous.  I suggest to say
explicitly that the gap must be at PT (although decode-coding doesn't
really blindly assume that, as you can see from its calls to
move_gap_both).

> +   If this is the same buffer as CODING->dst_object, CODING->src_pos must
> +   be negative.

I don't see the condition of the same buffer in the code.  What did I
miss?

> +   When the text is taken from the gap, it can't be at the beginning of
> +   the gap so that we can produce the decoded text at the beginning of
> +   the gap: this way, as the output grows, the input shrinks, so we only
> +   need to allocate enough space for `max(IN, OUT)` instead of `IN + OUT`.

I think this should also tell that decoding in this setup takes bytes
from encoded text at the end of the gap and inserts the decoded text
starting at PT, which is the same as the beginning of the gap.
Without saying that, the above paragraph might not be as clear as it
should be.

bug#36431: Crash in marker.c:337

[Eli Zaretskii]

> From: Stefan Monnier <monnier@iro.umontreal.ca>
> Cc: wl@gnu.org,  36431@debbugs.gnu.org
> Date: Wed, 03 Jul 2019 00:21:54 -0400
> 
> -      move_gap_both (PT, PT_BYTE);
> -      GAP_SIZE += inserted;
> -      ZV_BYTE -= inserted;
> -      Z_BYTE -= inserted;
> -      ZV -= inserted;
> -      Z -= inserted;
> +      if (ingap)
> +        { /* Text is at beginning of gap, move it to the end.  */
> +          memmove (GAP_END_ADDR - inserted, GPT_ADDR, inserted);
> +        }
> +      else
> +        { /* Text is inside the buffer; move it to end of the gap.  */
> +          move_gap_both (PT, PT_BYTE);
> +	  eassert (inserted == Z_BYTE - BEG_BYTE);
> +          GAP_SIZE += inserted;
> +          ZV = Z = GPT = BEG;
> +          ZV_BYTE = Z_BYTE = GPT_BYTE = BEG_BYTE;
> +          /* Now we are safe to change the buffer's multibyteness directly.  */
> +          bset_enable_multibyte_characters (current_buffer, multibyte);
> +        }
> +

Why did you prefer to use memmove instead of move_gap_both?  AFAIK the
latter actually does the former under the hood, but why expose that
implementation detail outside of insdel.c?

bug#36431: Crash in marker.c:337

[Werner LEMBERG]

[-- Attachment #1: Type: Text/Plain, Size: 354 bytes --]


> Here's an alternative patch which doesn't suffer from this problem
> but also eliminates the transiently-inconsistent multibyte buffer
> situation.

Thanks for the patch.  It partially works: One crash is avoided that I
get without it, but another crash appears every time I want to send an
e-mail with `mew'; see the attached backtrace.


    Werner

[-- Attachment #2: gdb.txt.xz --]
[-- Type: Application/Octet-Stream, Size: 6728 bytes --]

bug#36431: Crash in marker.c:337

[Eli Zaretskii]

> Date: Wed, 03 Jul 2019 08:20:23 +0200 (CEST)
> Cc: eliz@gnu.org, 36431@debbugs.gnu.org
> From: Werner LEMBERG <wl@gnu.org>
> 
> > Here's an alternative patch which doesn't suffer from this problem
> > but also eliminates the transiently-inconsistent multibyte buffer
> > situation.
> 
> Thanks for the patch.  It partially works: One crash is avoided that I
> get without it, but another crash appears every time I want to send an
> e-mail with `mew'; see the attached backtrace.

Werner,

PLEASE try to prepare a reproduction recipe as I suggested earlier.
Without that, debugging this problem is going to be very inefficient,
and we will not be able to tell for sure we fixed it even if you
report that your particular use case no longer triggers assertions.

TIA

P.S. I also think your sources are out of sync with the current
master, which is another factor that makes debugging harder and more
error-prone.  Can you update from master, please?

bug#36431: Crash in marker.c:337

[Werner LEMBERG]

> PLEASE try to prepare a reproduction recipe as I suggested earlier.
> Without that, debugging this problem is going to be very
> inefficient, and we will not be able to tell for sure we fixed it
> even if you report that your particular use case no longer triggers
> assertions.

I know, I know, however, this needs *a lot* of time which I don't have
right now, sorry.  Will try to come up with something in the future.

> P.S. I also think your sources are out of sync with the current
> master, which is another factor that makes debugging harder and more
> error-prone.  Can you update from master, please?

Nope, I'm *always* updating to master (this time it is commit
20c1406c), then applying the patch.


    Werner


PS: I do `git pull', apply the patch, then running `make && make
    install' from the top-level directory, re-using the stuff from
    previous compilations.  In case this isn't sufficient please tell
    me.

bug#36431: Crash in marker.c:337

[Eli Zaretskii]

> Date: Wed, 03 Jul 2019 08:46:21 +0200 (CEST)
> Cc: monnier@iro.umontreal.ca, 36431@debbugs.gnu.org
> From: Werner LEMBERG <wl@gnu.org>
> 
> > P.S. I also think your sources are out of sync with the current
> > master, which is another factor that makes debugging harder and more
> > error-prone.  Can you update from master, please?
> 
> Nope, I'm *always* updating to master (this time it is commit
> 20c1406c), then applying the patch.

Then maybe I got confused by the time sequence of changes Stefan made
in the repository and the patches he sent to you.  Sorry.

bug#36431: Crash in marker.c:337

[Stefan Monnier]

> Thanks for the patch.  It partially works: One crash is avoided that I
> get without it, but another crash appears every time I want to send an
> e-mail with `mew'; see the attached backtrace.

Thanks, that should be easy to fix,
I'll send another patch soon,


        Stefan

bug#36431: Crash in marker.c:337

[Stefan Monnier]

> (Note that it actually only uses the byte offset's numerical value.  I
> couldn't find any place where it actually uses the character offset in
> src_pos, it only checks its sign.)

The source is required to be unibyte, so src_pos and src_pos_byte have
to be equal at start and one of the two is redundant.

>> There are such design comments at various places.  Usually added
>> after the fact, sometimes added as part of a commit-reversal to make
>> sure someone else doesn't fall into the same trap ;-)
> Interesting.  Can you point me to a couple of such design comments?

Not off-hand, no, but I know I added such comments every once in a while.

>>     If CODING->src_object is a buffer, it must be the current buffer.
>>     In this case, if CODING->src_pos is positive, it is a position of
>>     the source text in the buffer, otherwise, the source text is in the
>> -   gap area of the buffer, and CODING->src_pos specifies the offset of
>> -   the text from GPT (which must be the same as PT).  If this is the
>> -   same buffer as CODING->dst_object, CODING->src_pos must be
>> -   negative.
>> +   gap area of the buffer, and CODING->src_pos specifies the
>> +   offset of the text from the end of the gap (which must be at PT).
>
> The "which must be at PT" part is ambiguous.  I suggest to say
> explicitly that the gap must be at PT

AFAICT that's exactly what it is saying, so I'm not sure what ambiguity
you're thinking of.

> (although decode-coding doesn't really blindly assume that, as you can
> see from its calls to move_gap_both).

[ FWIW, this part of the text is mostly preserved from the old text.  ]
I think the issue is that decode_coding's calls to move_gap_both *must*
be no-ops when the source is in the gap otherwise it'll destroy the
source-text.

>> +   If this is the same buffer as CODING->dst_object, CODING->src_pos must
>> +   be negative.
> I don't see the condition of the same buffer in the code.  What did I
> miss?

This one I don't know: I just preserved this part of the text.

>> +   When the text is taken from the gap, it can't be at the beginning of
>> +   the gap so that we can produce the decoded text at the beginning of
>> +   the gap: this way, as the output grows, the input shrinks, so we only
>> +   need to allocate enough space for `max(IN, OUT)` instead of `IN + OUT`.
>
> I think this should also tell that decoding in this setup takes bytes
> from encoded text at the end of the gap and inserts the decoded text
> starting at PT, which is the same as the beginning of the gap.

[ PT is both the beginning and the end of the gap ;-)  ]
OK, I'll clarify a bit, thanks.


        Stefan

bug#36431: Crash in marker.c:337

[Eli Zaretskii]

> From: Stefan Monnier <monnier@iro.umontreal.ca>
> Cc: wl@gnu.org,  36431@debbugs.gnu.org
> Date: Wed, 03 Jul 2019 12:19:22 -0400
> 
> > (Note that it actually only uses the byte offset's numerical value.  I
> > couldn't find any place where it actually uses the character offset in
> > src_pos, it only checks its sign.)
> 
> The source is required to be unibyte, so src_pos and src_pos_byte have
> to be equal at start and one of the two is redundant.

My point was that the code uses the sign of one of them and the value
of the other.

> >> +   gap area of the buffer, and CODING->src_pos specifies the
> >> +   offset of the text from the end of the gap (which must be at PT).
> >
> > The "which must be at PT" part is ambiguous.  I suggest to say
> > explicitly that the gap must be at PT
> 
> AFAICT that's exactly what it is saying, so I'm not sure what ambiguity
> you're thinking of.

The text could be interpreted as meaning that the end of the gap must
be at PT.  IOW, "which" could either allude to the gap or to the end
of the gap.

> > (although decode-coding doesn't really blindly assume that, as you can
> > see from its calls to move_gap_both).
> 
> [ FWIW, this part of the text is mostly preserved from the old text.  ]

I assumed that you wanted to clarify the comment, so preserving old
text sounds like a non-goal ;-)

> I think the issue is that decode_coding's calls to move_gap_both *must*
> be no-ops when the source is in the gap otherwise it'll destroy the
> source-text.

No, I meant the code there actually tests these conditions, and if
they are not true, it moves the gap as needed to make them true.

> >> +   If this is the same buffer as CODING->dst_object, CODING->src_pos must
> >> +   be negative.
> > I don't see the condition of the same buffer in the code.  What did I
> > miss?
> 
> This one I don't know: I just preserved this part of the text.

I don't think it's true.  CODING->src_pos must be negative if the
source text is in the gap, period.

> >> +   When the text is taken from the gap, it can't be at the beginning of
> >> +   the gap so that we can produce the decoded text at the beginning of
> >> +   the gap: this way, as the output grows, the input shrinks, so we only
> >> +   need to allocate enough space for `max(IN, OUT)` instead of `IN + OUT`.
> >
> > I think this should also tell that decoding in this setup takes bytes
> > from encoded text at the end of the gap and inserts the decoded text
> > starting at PT, which is the same as the beginning of the gap.
> 
> [ PT is both the beginning and the end of the gap ;-)  ]

Not in what I wrote, AFAICT.

bug#36431: Crash in marker.c:337

[Stefan Monnier]

> Thanks for the patch.  It partially works: One crash is avoided that I
> get without it, but another crash appears every time I want to send an
> e-mail with `mew'; see the attached backtrace.

I installed the patch below into master.

Eli said:
> I think we should do 2.

I also did that.


        Stefan