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