24.2.92 fix [Re: Crash caused by insert-file-contents, both trunk (bzr 111532) and 24.2.92 affected]

[Dmitry Antipov <dmantipov@yandex.ru>]

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

On 01/18/2013 11:34 PM, Eli Zaretskii wrote:

> I'm not sure I see how the gap size fails to be updated.  There's a
> call to make_gap just before read_non_regular is called.  Or did you
> mean GAP_SIZE?  If the latter, then the comments there explain why
> this is not done.

Argh. We fool itself with gap_size (src/fileio.c, 24.2.92):

   4098              /* Maybe make more room.  */
   4099              if (gap_size < trytry)
   4100                {
   4101                  make_gap (total - gap_size);
   4102                  gap_size = GAP_SIZE;           /* !!! here */
   4103                }

After that, local gap_size (e.g. amount of bytes which may be used to
read next chunk from the file) is GAP_SIZE - inserted, not GAP_SIZE.

> But until GAP_SIZE and ZV are updated, the inserted text is not
> really part of the buffer, right?  So what is the problem here?

Here is the original code (src/fileio.c, 24.2.92):

   4169    /* Make the text read part of the buffer.  */
   4170    GAP_SIZE -= inserted;
   4171    GPT      += inserted;
   4172    GPT_BYTE += inserted;
   4173    ZV       += inserted;
   4174    ZV_BYTE  += inserted;
   4175    Z        += inserted;
   4176    Z_BYTE   += inserted;
   4177                                                                    /* !!! `inserted' bytes becomes "really inserted" */
   4178    if (GAP_SIZE > 0)
   4179      /* Put an anchor to ensure multi-byte form ends at gap.  */
   4180      *GPT_ADDR = 0;
   4181
   4182    emacs_close (fd);
   4183
   4184    /* Discard the unwind protect for closing the file.  */
   4185    specpdl_ptr--;
   4186
   4187    if (how_much < 0)
   4188      error ("IO error reading %s: %s",                             /* error leaves `inserted' bytes not decoded !!! */
   4189             SDATA (orig_filename), emacs_strerror (errno));

Attached is the fix for 24.2.92, and I believe that this is important
for the next pretest. For trunk, I'll revert 111547 and do the similar fix.

Dmitry


[-- Attachment #2: insert_file_contents.patch --]
[-- Type: text/plain, Size: 2381 bytes --]

diff -x '*.o' -ur ../.orig-emacs-24.2.92/src/ChangeLog src/ChangeLog
--- ../.orig-emacs-24.2.92/src/ChangeLog	2013-01-05 23:06:21.000000000 +0400
+++ src/ChangeLog	2013-01-21 12:48:58.244932711 +0400
@@ -1,3 +1,13 @@
+2013-01-21  Dmitry Antipov  <dmantipov@yandex.ru>
+
+	Fix crash when inserting data from non-regular files.  See
+	http://lists.gnu.org/archive/html/emacs-devel/2013-01/msg00406.html
+	for the error description produced by valgrind.
+	* fileio.c (Finsert_file_contents): Adjust gap_size correctly.
+	Do not signal an I/O error too early and so do not leave not yet
+	decoded characters in a buffer, which was the reason of redisplay
+	crash.  Adjust comment.
+
 2013-01-05  Eli Zaretskii  <eliz@gnu.org>
 
 	* xdisp.c (dump_glyph): Align glyph data better.  Use "pD" instead
diff -x '*.o' -ur ../.orig-emacs-24.2.92/src/fileio.c src/fileio.c
--- ../.orig-emacs-24.2.92/src/fileio.c	2013-01-02 00:37:17.000000000 +0400
+++ src/fileio.c	2013-01-21 12:31:51.405273880 +0400
@@ -4098,8 +4098,8 @@
 	    /* Maybe make more room.  */
 	    if (gap_size < trytry)
 	      {
-		make_gap (total - gap_size);
-		gap_size = GAP_SIZE;
+		make_gap (trytry - gap_size);
+		gap_size = GAP_SIZE - inserted;
 	      }
 
 	    /* Read from the file, capturing `quit'.  When an
@@ -4152,8 +4152,9 @@
       }
   }
 
-  /* Now we have read all the file data into the gap.
-     If it was empty, undo marking the buffer modified.  */
+  /* Now we have either read all the file data into the gap,
+     or stop reading on I/O error or quit.  If nothing was
+     read, undo marking the buffer modified.  */
 
   if (inserted == 0)
     {
@@ -4166,6 +4167,15 @@
   else
     Vdeactivate_mark = Qt;
 
+  emacs_close (fd);
+
+  /* Discard the unwind protect for closing the file.  */
+  specpdl_ptr--;
+
+  if (how_much < 0)
+    error ("IO error reading %s: %s",
+	   SDATA (orig_filename), emacs_strerror (errno));
+
   /* Make the text read part of the buffer.  */
   GAP_SIZE -= inserted;
   GPT      += inserted;
@@ -4179,15 +4189,6 @@
     /* Put an anchor to ensure multi-byte form ends at gap.  */
     *GPT_ADDR = 0;
 
-  emacs_close (fd);
-
-  /* Discard the unwind protect for closing the file.  */
-  specpdl_ptr--;
-
-  if (how_much < 0)
-    error ("IO error reading %s: %s",
-	   SDATA (orig_filename), emacs_strerror (errno));
-
  notfound:
 
   if (NILP (coding_system))