Re: [Bug-cpio] wrong handling of write(2) errors

[Sergey Poznyakoff]

Vasiliy Kulikov <address@hidden> ha escrit:

> The functions disk_empty_output_buffer() and sparse_write() don't
> properly check write(2) return code, which can lead to data loss.

Thanks for reporting.  Sparse_write needed a good rewrite anyway, so
I installed the following patch.

Regards,
Sergey

>From 268310bcfc693e0cd19f0033597cbe58b38279a1 Mon Sep 17 00:00:00 2001
From: Sergey Poznyakoff <address@hidden>
Date: Mon, 19 Sep 2011 00:37:43 +0300
Subject: [PATCH] Fix error handling in disk_empty_output_buffer and sparse_write

* src/extern.h (delayed_seek_count): Remove.
(disk_empty_output_buffer): Change signature.
* src/util.c (disk_empty_output_buffer): Take two arguments.
Correctly handle partial writes (errno is not meaningful).
(delayed_seek_count): Remove variable.
(sparse_write): Change return type and signature.  Rewrite.
Return number actual number of bytes written or -1 on error.
Check returns from lseek and write.
* src/copyin.c (copyin_regular_file): Call disk_empty_output_buffer
with flush=true before closing the file.
* src/copypass.c (process_copy_pass): Likewise.
---
 src/copyin.c   |   12 +----
 src/copypass.c |   13 +----
 src/extern.h   |    3 +-
 src/util.c     |  181 ++++++++++++++++++++++++++-----------------------------
 4 files changed, 90 insertions(+), 119 deletions(-)

diff --git a/src/copyin.c b/src/copyin.c
index 22e33dc..3ab5dac 100644
--- a/src/copyin.c
+++ b/src/copyin.c
@@ -518,7 +518,7 @@ copyin_regular_file (struct cpio_file_stat* file_hdr, int 
in_file_des)
               file_hdr->c_name);
     }
   copy_files_tape_to_disk (in_file_des, out_file_des, file_hdr->c_filesize);
-  disk_empty_output_buffer (out_file_des);
+  disk_empty_output_buffer (out_file_des, true);
   
   if (to_stdout_option)
     {
@@ -532,16 +532,6 @@ copyin_regular_file (struct cpio_file_stat* file_hdr, int 
in_file_des)
       return;
     }
       
-  /* Debian hack to fix a bug in the --sparse option.
-     This bug has been reported to
-     "address@hidden".  (96/7/10) -BEM */
-  if (delayed_seek_count > 0)
-    {
-      lseek (out_file_des, delayed_seek_count-1, SEEK_CUR);
-      write (out_file_des, "", 1);
-      delayed_seek_count = 0;
-    }
-
   set_perms (out_file_des, file_hdr);
 
   if (close (out_file_des) < 0)
diff --git a/src/copypass.c b/src/copypass.c
index 5b1d594..a3ccb71 100644
--- a/src/copypass.c
+++ b/src/copypass.c
@@ -200,17 +200,8 @@ process_copy_pass ()
                }
 
              copy_files_disk_to_disk (in_file_des, out_file_des, 
in_file_stat.st_size, input_name.ds_string);
-             disk_empty_output_buffer (out_file_des);
-             /* Debian hack to fix a bug in the --sparse option.
-                 This bug has been reported to
-                 "address@hidden".  (96/7/10) -BEM */
-             if (delayed_seek_count > 0)
-               {
-                 lseek (out_file_des, delayed_seek_count-1, SEEK_CUR);
-                 write (out_file_des, "", 1);
-                 delayed_seek_count = 0;
-               }
-
+             disk_empty_output_buffer (out_file_des, true);
+             
              set_copypass_perms (out_file_des,
                                  output_name.ds_string, &in_file_stat);
 
diff --git a/src/extern.h b/src/extern.h
index c25a6ef..be329ae 100644
--- a/src/extern.h
+++ b/src/extern.h
@@ -76,7 +76,6 @@ extern int archive_des;
 extern char *archive_name;
 extern char *rsh_command_option;
 extern unsigned long crc;
-extern int delayed_seek_count;
 #ifdef DEBUG_CPIO
 extern int debug_flag;
 #endif
@@ -157,7 +156,7 @@ char *parse_user_spec (char *name, uid_t *uid, gid_t *gid,
 
 /* util.c */
 void tape_empty_output_buffer (int out_des);
-void disk_empty_output_buffer (int out_des);
+void disk_empty_output_buffer (int out_des, bool flush);
 void swahw_array (char *ptr, int count);
 void tape_buffered_write (char *in_buf, int out_des, off_t num_bytes);
 void tape_buffered_read (char *in_buf, int in_des, off_t num_bytes);
diff --git a/src/util.c b/src/util.c
index 7c6db52..22725e4 100644
--- a/src/util.c
+++ b/src/util.c
@@ -1,6 +1,6 @@
 /* util.c - Several utility routines for cpio.
-   Copyright (C) 1990, 1991, 1992, 2001, 2004, 2006, 2007, 2010 Free
-   Software Foundation, Inc.
+   Copyright (C) 1990, 1991, 1992, 2001, 2004, 2006, 2007, 2010,
+   2011 Free Software Foundation, Inc.
 
    This program is free software; you can redistribute it and/or modify
    it under the terms of the GNU General Public License as published by
@@ -100,7 +100,7 @@ tape_empty_output_buffer (int out_des)
   output_size = 0;
 }
 
-static int sparse_write (int fildes, char *buf, unsigned int nbyte);
+static ssize_t sparse_write (int fildes, char *buf, size_t nbyte, bool flush);
 
 /* Write `output_size' bytes of `output_buffer' to file
    descriptor OUT_DES and reset `output_size' and `out_buff'.
@@ -113,9 +113,9 @@ static int sparse_write (int fildes, char *buf, unsigned 
int nbyte);
    insure this.  */
 
 void
-disk_empty_output_buffer (int out_des)
+disk_empty_output_buffer (int out_des, bool flush)
 {
-  int bytes_written;
+  ssize_t bytes_written;
 
   if (swapping_halfwords || swapping_bytes)
     {
@@ -136,13 +136,16 @@ disk_empty_output_buffer (int out_des)
     }
 
   if (sparse_flag)
-    bytes_written = sparse_write (out_des, output_buffer, output_size);
+    bytes_written = sparse_write (out_des, output_buffer, output_size, flush);
   else
     bytes_written = write (out_des, output_buffer, output_size);
 
   if (bytes_written != output_size)
     {
-      error (1, errno, _("write error"));
+      if (bytes_written == -1)
+       error (1, errno, _("write error"));
+      else
+       error (1, 0, _("write error: partial write"));
     }
   output_bytes += output_size;
   out_buff = output_buffer;
@@ -275,7 +278,7 @@ disk_buffered_write (char *in_buf, int out_des, off_t 
num_bytes)
     {
       space_left = DISK_IO_BLOCK_SIZE - output_size;
       if (space_left == 0)
-       disk_empty_output_buffer (out_des);
+       disk_empty_output_buffer (out_des, false);
       else
        {
          if (bytes_left < space_left)
@@ -1111,107 +1114,95 @@ buf_all_zeros (char *buf, int bufsize)
   return 1;
 }
 
-int delayed_seek_count = 0;
+/* Write NBYTE bytes from BUF to file descriptor FILDES, trying to
+   create holes instead of writing blockfuls of zeros.
+   
+   Return the number of bytes written (including bytes in zero
+   regions) on success, -1 on error.
 
-/* Write NBYTE bytes from BUF to remote tape connection FILDES.
-   Return the number of bytes written on success, -1 on error.  */
+   If FLUSH is set, make sure the trailing zero region is flushed
+   on disk.
+*/
 
-static int
-sparse_write (int fildes, char *buf, unsigned int nbyte)
+static ssize_t
+sparse_write (int fildes, char *buf, size_t nbytes, bool flush)
 {
-  int complete_block_count;
-  int leftover_bytes_count;
-  int seek_count;
-  int write_count;
-  char *cur_write_start;
-  int lseek_rc;
-  int write_rc;
-  int i;
-  enum { begin, in_zeros, not_in_zeros } state;
-
-  complete_block_count = nbyte / DISKBLOCKSIZE;
-  leftover_bytes_count = nbyte % DISKBLOCKSIZE;
-
-  if (delayed_seek_count != 0)
-    state = in_zeros;
-  else
-    state = begin;
+  size_t nwritten = 0;
+  ssize_t n;
+  char *start_ptr = buf;
 
-  seek_count = delayed_seek_count;
+  static off_t delayed_seek_count = 0;
+  off_t seek_count = 0;
 
-  for (i = 0; i < complete_block_count; ++i)
+  enum { begin, in_zeros, not_in_zeros } state =
+                          delayed_seek_count ? in_zeros : begin;
+  
+  while (nbytes)
     {
-      switch (state)
+      size_t rest = nbytes;
+
+      if (rest < DISKBLOCKSIZE)
+       /* Force write */
+       state = not_in_zeros;
+      else
        {
-         case begin :
-           if (buf_all_zeros (buf, DISKBLOCKSIZE))
-             {
-               seek_count = DISKBLOCKSIZE;
-               state = in_zeros;
-             }
-           else
-             {
-               cur_write_start = buf;
-               write_count = DISKBLOCKSIZE;
-               state = not_in_zeros;
-             }
-           buf += DISKBLOCKSIZE;
-           break;
-           
-         case in_zeros :
-           if (buf_all_zeros (buf, DISKBLOCKSIZE))
-             {
-               seek_count += DISKBLOCKSIZE;
-             }
-           else
-             {
-               lseek (fildes, seek_count, SEEK_CUR);
-               cur_write_start = buf;
-               write_count = DISKBLOCKSIZE;
-               state = not_in_zeros;
-             }
-           buf += DISKBLOCKSIZE;
-           break;
-           
-         case not_in_zeros :
-           if (buf_all_zeros (buf, DISKBLOCKSIZE))
-             {
-               write_rc = write (fildes, cur_write_start, write_count);
-               seek_count = DISKBLOCKSIZE;
-               state = in_zeros;
-             }
-           else
-             {
-               write_count += DISKBLOCKSIZE;
-             }
-           buf += DISKBLOCKSIZE;
-           break;
+         if (buf_all_zeros (buf, rest))
+           {
+             if (state == not_in_zeros)
+               {
+                 ssize_t bytes = buf - start_ptr + rest;
+                 
+                 n = write (fildes, start_ptr, bytes);
+                 if (n == -1)
+                   return -1;
+                 nwritten += n;
+                 if (n < bytes)
+                   return nwritten + seek_count;
+                 start_ptr = NULL;
+               }
+             else
+               seek_count += rest;
+             state = in_zeros;
+           }
+         else
+           {
+             seek_count += delayed_seek_count;
+             if (lseek (fildes, seek_count, SEEK_CUR) == -1)
+               return -1;
+             delayed_seek_count = seek_count = 0;
+             state = not_in_zeros;
+             start_ptr = buf;
+           }
        }
+      buf += rest;
+      nbytes -= rest;
     }
 
-  switch (state)
+  if (state != in_zeros)
     {
-      case begin :
-      case in_zeros :
-       delayed_seek_count = seek_count;
-       break;
-       
-      case not_in_zeros :
-       write_rc = write (fildes, cur_write_start, write_count);
-       delayed_seek_count = 0;
-       break;
+      seek_count += delayed_seek_count;
+      if (seek_count && lseek (fildes, seek_count, SEEK_CUR) == -1)
+       return -1;
+      delayed_seek_count = seek_count = 0;
+
+      n = write (fildes, start_ptr, buf - start_ptr);
+      if (n == -1)
+       return n;
+      nwritten += n;
     }
+  delayed_seek_count += seek_count;
 
-  if (leftover_bytes_count != 0)
+  if (flush && delayed_seek_count)
     {
-      if (delayed_seek_count != 0)
-       {
-         lseek_rc = lseek (fildes, delayed_seek_count, SEEK_CUR);
-         delayed_seek_count = 0;
-       }
-      write_rc = write (fildes, buf, leftover_bytes_count);
-    }
-  return nbyte;
+      if (lseek (fildes, delayed_seek_count - 1, SEEK_CUR) == -1)
+       return -1;
+      n = write (fildes, "", 1);
+      if (n != 1)
+       return n;
+      delayed_seek_count = 0;
+    }      
+  
+  return nwritten + seek_count;
 }
 
 #define CPIO_UID(uid) (set_owner_flag ? set_owner : (uid))
-- 
1.7.3.2