[Emacs-diffs] /srv/bzr/emacs/trunk r111547: Fix crash when inserting data from non-regular files. See

[Dmitry Antipov]

------------------------------------------------------------
revno: 111547
committer: Dmitry Antipov <address@hidden>
branch nick: trunk
timestamp: Fri 2013-01-18 10:32:12 +0400
message:
  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 (read_non_regular): Rename to read_contents.
  Free Lisp_Save_Value object used to pass parameters.
  (read_non_regular_quit): Rename to read_contents_quit.
  (Finsert_file_contents): Redesign internal file reading loop to adjust
  gap and end positions after each read and so help make_gap to work
  properly.  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.  Use list2 to build return value.  Adjust comments.
modified:
  src/ChangeLog
  src/fileio.c

=== modified file 'src/ChangeLog'
--- a/src/ChangeLog     2013-01-18 05:12:08 +0000
+++ b/src/ChangeLog     2013-01-18 06:32:12 +0000
@@ -1,3 +1,17 @@
+2013-01-18  Dmitry Antipov  <address@hidden>
+
+       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 (read_non_regular): Rename to read_contents.
+       Free Lisp_Save_Value object used to pass parameters.
+       (read_non_regular_quit): Rename to read_contents_quit.
+       (Finsert_file_contents): Redesign internal file reading loop to adjust
+       gap and end positions after each read and so help make_gap to work
+       properly.  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.  Use list2 to build return value.  Adjust comments.
+
 2013-01-17  Paul Eggert  <address@hidden>
 
        Close a race when statting and reading files (Bug#13149).

=== modified file 'src/fileio.c'
--- a/src/fileio.c      2013-01-18 05:12:08 +0000
+++ b/src/fileio.c      2013-01-18 06:32:12 +0000
@@ -3408,13 +3408,13 @@
   return Qnil;
 }
 
-/* Read from a non-regular file.  STATE is a Lisp_Save_Value
+/* Check quit and read from the file.  STATE is a Lisp_Save_Value
    object where slot 0 is the file descriptor, slot 1 specifies
    an offset to put the read bytes, and slot 2 is the maximum
    amount of bytes to read.  Value is the number of bytes read.  */
 
 static Lisp_Object
-read_non_regular (Lisp_Object state)
+read_contents (Lisp_Object state)
 {
   int nbytes;
 
@@ -3425,15 +3425,15 @@
                        + XSAVE_INTEGER (state, 1)),
                       XSAVE_INTEGER (state, 2));
   immediate_quit = 0;
+  /* Fast recycle this object for the likely next call.  */
+  free_misc (state);
   return make_number (nbytes);
 }
 
-
-/* Condition-case handler used when reading from non-regular files
-   in insert-file-contents.  */
+/* Condition-case handler used when reading files in insert-file-contents.  */
 
 static Lisp_Object
-read_non_regular_quit (Lisp_Object ignore)
+read_contents_quit (Lisp_Object ignore)
 {
   return Qnil;
 }
@@ -3505,7 +3505,7 @@
   Lisp_Object p;
   ptrdiff_t total = 0;
   bool not_regular = 0;
-  int save_errno = 0;
+  int save_errno = 0, read_errno = 0;
   char read_buf[READ_BUF_SIZE];
   struct coding_system coding;
   char buffer[1 << 14];
@@ -4195,88 +4195,72 @@
                           Fcons (orig_filename, Qnil));
     }
 
-  /* In the following loop, HOW_MUCH contains the total bytes read so
-     far for a regular file, and not changed for a special file.  But,
-     before exiting the loop, it is set to a negative value if I/O
-     error occurs.  */
+  /* In the following loop, HOW_MUCH contains the total bytes read
+     so far for a regular file, and not changed for a special file.  */
   how_much = 0;
 
   /* Total bytes inserted.  */
   inserted = 0;
 
-  /* Here, we don't do code conversion in the loop.  It is done by
-     decode_coding_gap after all data are read into the buffer.  */
-  {
-    ptrdiff_t gap_size = GAP_SIZE;
-
-    while (how_much < total)
-      {
-       /* try is reserved in some compilers (Microsoft C) */
-       ptrdiff_t trytry = min (total - how_much, READ_BUF_SIZE);
-       ptrdiff_t this;
-
-       if (not_regular)
-         {
-           Lisp_Object nbytes;
-
-           /* Maybe make more room.  */
-           if (gap_size < trytry)
-             {
-               make_gap (total - gap_size);
-               gap_size = GAP_SIZE;
-             }
-
-           /* Read from the file, capturing `quit'.  When an
-              error occurs, end the loop, and arrange for a quit
-              to be signaled after decoding the text we read.  */
-           nbytes = internal_condition_case_1
-             (read_non_regular,
-              make_save_value ("iii", (ptrdiff_t) fd, inserted, trytry),
-              Qerror, read_non_regular_quit);
-
-           if (NILP (nbytes))
-             {
-               read_quit = 1;
-               break;
-             }
-
-           this = XINT (nbytes);
-         }
-       else
-         {
-           /* Allow quitting out of the actual I/O.  We don't make text
-              part of the buffer until all the reading is done, so a C-g
-              here doesn't do any harm.  */
-           immediate_quit = 1;
-           QUIT;
-           this = emacs_read (fd,
-                              ((char *) BEG_ADDR + PT_BYTE - BEG_BYTE
-                               + inserted),
-                              trytry);
-           immediate_quit = 0;
-         }
-
-       if (this <= 0)
-         {
-           how_much = this;
-           break;
-         }
-
-       gap_size -= this;
-
-       /* For a regular file, where TOTAL is the real size,
-          count HOW_MUCH to compare with it.
-          For a special file, where TOTAL is just a buffer size,
-          so don't bother counting in HOW_MUCH.
-          (INSERTED is where we count the number of characters inserted.)  */
-       if (! not_regular)
-         how_much += this;
-       inserted += this;
-      }
-  }
-
-  /* Now we have read all the file data into the gap.
-     If it was empty, undo marking the buffer modified.  */
+  /* Here we don't do code conversion in the loop.  It is done by
+     decode_coding_gap after all data are read into the buffer, or
+     reading loop is interrupted with quit or due to I/O error.  */
+
+  while (how_much < total)
+    {
+      ptrdiff_t nread, maxread = min (total - how_much, READ_BUF_SIZE);
+      Lisp_Object result;
+
+      /* For a special file, gap is enlarged as we read,
+        so GAP_SIZE should be checked every time.  */
+      if (not_regular && (GAP_SIZE < maxread))
+       make_gap (maxread - GAP_SIZE);
+
+      /* Read from the file, capturing `quit'.  */
+      result = internal_condition_case_1
+       (read_contents,
+        make_save_value ("iii", (ptrdiff_t) fd, inserted, maxread),
+        Qerror, read_contents_quit);
+      if (NILP (result))
+       {
+         /* Quit is signaled.  End the loop and arrange
+            real quit after decoding the text we read.  */
+         read_quit = 1;
+         break;
+       }
+      nread = XINT (result);
+      if (nread <= 0)
+       {
+         /* End of file or I/O error.  End the loop and
+            save error code in case of I/O error.  */
+         if (nread < 0)
+           read_errno = errno;
+         break;
+       }
+
+      /* Adjust gap and end positions.  */
+      GAP_SIZE -= nread;
+      GPT += nread;
+      ZV += nread;
+      Z += nread;
+      GPT_BYTE += nread;
+      ZV_BYTE += nread;
+      Z_BYTE += nread;
+      if (GAP_SIZE > 0)
+       *(GPT_ADDR) = 0;
+
+      /* For a regular file, where TOTAL is the real size, count HOW_MUCH to
+        compare with it.  For a special file, where TOTAL is just a buffer
+        size, don't bother counting in HOW_MUCH, but always accumulate the
+        number of bytes read in INSERTED.  */
+      if (!not_regular)
+       how_much += nread;
+      inserted += nread;
+    }
+
+  /* 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)
     {
@@ -4289,28 +4273,11 @@
   else
     Vdeactivate_mark = Qt;
 
-  /* 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;
-
   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))
@@ -4599,14 +4566,17 @@
       report_file_error ("Opening input file", Fcons (orig_filename, Qnil));
     }
 
+  /* There was an error reading file.  */
+  if (read_errno)
+    error ("IO error reading %s: %s",
+          SDATA (orig_filename), emacs_strerror (read_errno));
+
+  /* Quit was signaled.  */
   if (read_quit)
     Fsignal (Qquit, Qnil);
 
-  /* ??? Retval needs to be dealt with in all cases consistently.  */
   if (NILP (val))
-    val = Fcons (orig_filename,
-                Fcons (make_number (inserted),
-                       Qnil));
+    val = list2 (orig_filename, make_number (inserted));
 
   RETURN_UNGCPRO (unbind_to (count, val));
 }