Re: [PATCH] util: Detect more I/O errors

[Steve McIntyre]

On Wed, Feb 27, 2019 at 09:10:08AM +0000, Colin Watson wrote:
>Many of GRUB's utilities don't check anywhere near all the possible
>write errors.  For example, if grub-install runs out of space when
>copying a file, it won't notice.  There were missing checks for the
>return values of write, fflush, fsync, and close (or the equivalents on
>other OSes), all of which must be checked.
>
>I tried to be consistent with the existing logging practices of the
>various hostdisk implementations, but they weren't entirely consistent
>to start with so I used my judgement.  The result at least looks
>reasonable on GNU/Linux when I provoke a write error:
>
>  Installing for x86_64-efi platform.
>  grub-install: error: cannot copy 
> `/usr/lib/grub/x86_64-efi-signed/grubx64.efi.signed' to 
> `/boot/efi/EFI/debian/grubx64.efi': No space left on device.
>
>There are more missing checks in other utilities, but this should fix
>the most critical ones.
>
>Fixes Debian bug #922741.
>---
> grub-core/osdep/aros/hostdisk.c    | 38 ++++++++++++++++++------------
> grub-core/osdep/unix/hostdisk.c    | 18 +++++++-------
> grub-core/osdep/windows/hostdisk.c | 37 ++++++++++++++++++++++-------
> include/grub/emu/hostfile.h        |  4 ++--
> include/grub/emu/misc.h            |  2 +-
> util/editenv.c                     |  3 ++-
> util/grub-editenv.c                |  3 ++-
> util/grub-install-common.c         | 16 +++++++++----
> util/grub-mkimage.c                |  8 +++++--
> util/setup.c                       |  6 +++--
> 10 files changed, 90 insertions(+), 45 deletions(-)

LGTM

Reviewed-by: Steve McIntyre <address@hidden>

>
>diff --git a/grub-core/osdep/aros/hostdisk.c b/grub-core/osdep/aros/hostdisk.c
>index 7d99b54b8..2be654ca3 100644
>--- a/grub-core/osdep/aros/hostdisk.c
>+++ b/grub-core/osdep/aros/hostdisk.c
>@@ -439,36 +439,44 @@ grub_util_get_fd_size (grub_util_fd_t fd,
>   return -1;
> }
> 
>-void
>+int
> grub_util_fd_close (grub_util_fd_t fd)
> {
>   switch (fd->type)
>     {
>     case GRUB_UTIL_FD_FILE:
>-      close (fd->fd);
>-      return;
>+      return close (fd->fd);
>     case GRUB_UTIL_FD_DISK:
>       CloseDevice ((struct IORequest *) fd->ioreq);
>       DeleteIORequest((struct IORequest *) fd->ioreq);
>       DeleteMsgPort (fd->mp);
>-      return;
>+      return 0;
>     }
>+  return 0;
> }
> 
> static int allow_fd_syncs = 1;
> 
>-static void
>+static int
> grub_util_fd_sync_volume (grub_util_fd_t fd)
> {
>+  LONG err;
>+
>   fd->ioreq->iotd_Req.io_Command = CMD_UPDATE;
>   fd->ioreq->iotd_Req.io_Length = 0;
>   fd->ioreq->iotd_Req.io_Data = 0;
>   fd->ioreq->iotd_Req.io_Offset = 0;
>   fd->ioreq->iotd_Req.io_Actual = 0;
>-  DoIO ((struct IORequest *) fd->ioreq);
>+  err = DoIO ((struct IORequest *) fd->ioreq);
>+  if (err)
>+    {
>+      grub_util_info ("I/O failed with error %d, IoErr=%d", (int)err, (int) 
>IoErr ());
>+      return -1;
>+    }
>+  return 0;
> }
> 
>-void
>+int
> grub_util_fd_sync (grub_util_fd_t fd)
> {
>   if (allow_fd_syncs)
>@@ -476,22 +484,22 @@ grub_util_fd_sync (grub_util_fd_t fd)
>       switch (fd->type)
>       {
>       case GRUB_UTIL_FD_FILE:
>-        fsync (fd->fd);
>-        return;
>+        return fsync (fd->fd);
>       case GRUB_UTIL_FD_DISK:
>-        grub_util_fd_sync_volume (fd);
>-        return;
>+        return grub_util_fd_sync_volume (fd);
>       }
>     }
>+  return 0;
> }
> 
>-void
>+int
> grub_util_file_sync (FILE *f)
> {
>-  fflush (f);
>+  if (fflush (f) != 0)
>+    return -1;
>   if (!allow_fd_syncs)
>-    return;
>-  fsync (fileno (f));
>+    return 0;
>+  return fsync (fileno (f));
> }
> 
> void
>diff --git a/grub-core/osdep/unix/hostdisk.c b/grub-core/osdep/unix/hostdisk.c
>index 5450cf416..91150969b 100644
>--- a/grub-core/osdep/unix/hostdisk.c
>+++ b/grub-core/osdep/unix/hostdisk.c
>@@ -177,20 +177,22 @@ grub_util_fd_strerror (void)
> 
> static int allow_fd_syncs = 1;
> 
>-void
>+int
> grub_util_fd_sync (grub_util_fd_t fd)
> {
>   if (allow_fd_syncs)
>-    fsync (fd);
>+    return fsync (fd);
>+  return 0;
> }
> 
>-void
>+int
> grub_util_file_sync (FILE *f)
> {
>-  fflush (f);
>+  if (fflush (f) != 0)
>+    return -1;
>   if (!allow_fd_syncs)
>-    return;
>-  fsync (fileno (f));
>+    return 0;
>+  return fsync (fileno (f));
> }
> 
> void
>@@ -199,10 +201,10 @@ grub_util_disable_fd_syncs (void)
>   allow_fd_syncs = 0;
> }
> 
>-void
>+int
> grub_util_fd_close (grub_util_fd_t fd)
> {
>-  close (fd);
>+  return close (fd);
> }
> 
> char *
>diff --git a/grub-core/osdep/windows/hostdisk.c 
>b/grub-core/osdep/windows/hostdisk.c
>index 85507af88..355100789 100644
>--- a/grub-core/osdep/windows/hostdisk.c
>+++ b/grub-core/osdep/windows/hostdisk.c
>@@ -275,11 +275,18 @@ grub_util_fd_write (grub_util_fd_t fd, const char *buf, 
>size_t len)
> 
> static int allow_fd_syncs = 1;
> 
>-void
>+int
> grub_util_fd_sync (grub_util_fd_t fd)
> {
>   if (allow_fd_syncs)
>-    FlushFileBuffers (fd);
>+    {
>+      if (!FlushFileBuffers (fd))
>+      {
>+        grub_util_info ("flush err %x", (int) GetLastError ());
>+        return -1;
>+      }
>+    }
>+  return 0;
> }
> 
> void
>@@ -288,10 +295,15 @@ grub_util_disable_fd_syncs (void)
>   allow_fd_syncs = 0;
> }
> 
>-void
>+int
> grub_util_fd_close (grub_util_fd_t fd)
> {
>-  CloseHandle (fd);
>+  if (!CloseHandle (fd))
>+    {
>+      grub_util_info ("close err %x", (int) GetLastError ());
>+      return -1;
>+    }
>+  return 0;
> }
> 
> const char *
>@@ -620,16 +632,25 @@ grub_util_fopen (const char *path, const char *mode)
>   return ret;
> }
> 
>-void
>+int
> grub_util_file_sync (FILE *f)
> {
>   HANDLE hnd;
> 
>-  fflush (f);
>+  if (fflush (f) != 0)
>+    {
>+      grub_util_info ("fflush err %x", (int) GetLastError ());
>+      return -1;
>+    }
>   if (!allow_fd_syncs)
>-    return;
>+    return 0;
>   hnd = (HANDLE) _get_osfhandle (fileno (f));
>-  FlushFileBuffers (hnd);
>+  if (!FlushFileBuffers (hnd))
>+    {
>+      grub_util_info ("flush err %x", (int) GetLastError ());
>+      return -1;
>+    }
>+  return 0;
> }
> 
> int
>diff --git a/include/grub/emu/hostfile.h b/include/grub/emu/hostfile.h
>index 8e37d5acb..cfb1e2b56 100644
>--- a/include/grub/emu/hostfile.h
>+++ b/include/grub/emu/hostfile.h
>@@ -47,11 +47,11 @@ grub_util_fd_t
> EXPORT_FUNC(grub_util_fd_open) (const char *os_dev, int flags);
> const char *
> EXPORT_FUNC(grub_util_fd_strerror) (void);
>-void
>+int
> grub_util_fd_sync (grub_util_fd_t fd);
> void
> grub_util_disable_fd_syncs (void);
>-void
>+int
> EXPORT_FUNC(grub_util_fd_close) (grub_util_fd_t fd);
> 
> grub_uint64_t
>diff --git a/include/grub/emu/misc.h b/include/grub/emu/misc.h
>index df6085bcb..59b8b35fc 100644
>--- a/include/grub/emu/misc.h
>+++ b/include/grub/emu/misc.h
>@@ -73,6 +73,6 @@ FILE *
> grub_util_fopen (const char *path, const char *mode);
> #endif
> 
>-void grub_util_file_sync (FILE *f);
>+int grub_util_file_sync (FILE *f);
> 
> #endif /* GRUB_EMU_MISC_H */
>diff --git a/util/editenv.c b/util/editenv.c
>index c6f8d2298..eb2d0c03a 100644
>--- a/util/editenv.c
>+++ b/util/editenv.c
>@@ -55,7 +55,8 @@ grub_util_create_envblk_file (const char *name)
>                    strerror (errno));
> 
> 
>-  grub_util_file_sync (fp);
>+  if (grub_util_file_sync (fp) < 0)
>+    grub_util_error (_("cannot sync `%s': %s"), namenew, strerror (errno));
>   free (buf);
>   fclose (fp);
> 
>diff --git a/util/grub-editenv.c b/util/grub-editenv.c
>index 118e89fe5..f3662c95b 100644
>--- a/util/grub-editenv.c
>+++ b/util/grub-editenv.c
>@@ -197,7 +197,8 @@ write_envblk (const char *name, grub_envblk_t envblk)
>     grub_util_error (_("cannot write to `%s': %s"), name,
>                    strerror (errno));
> 
>-  grub_util_file_sync (fp);
>+  if (grub_util_file_sync (fp) < 0)
>+    grub_util_error (_("cannot sync `%s': %s"), name, strerror (errno));
>   fclose (fp);
> }
> 
>diff --git a/util/grub-install-common.c b/util/grub-install-common.c
>index 0a7d68b9b..ca0ac612a 100644
>--- a/util/grub-install-common.c
>+++ b/util/grub-install-common.c
>@@ -112,11 +112,16 @@ grub_install_copy_file (const char *src,
>       r = grub_util_fd_read (in, grub_install_copy_buffer, 
> GRUB_INSTALL_COPY_BUFFER_SIZE);
>       if (r <= 0)
>       break;
>-      grub_util_fd_write (out, grub_install_copy_buffer, r);
>+      r = grub_util_fd_write (out, grub_install_copy_buffer, r);
>+      if (r <= 0)
>+      break;
>     }
>-  grub_util_fd_sync (out);
>-  grub_util_fd_close (in);
>-  grub_util_fd_close (out);
>+  if (grub_util_fd_sync (out) < 0)
>+    r = -1;
>+  if (grub_util_fd_close (in) < 0)
>+    r = -1;
>+  if (grub_util_fd_close (out) < 0)
>+    r = -1;
> 
>   if (r < 0)
>     grub_util_error (_("cannot copy `%s' to `%s': %s"),
>@@ -526,7 +531,8 @@ grub_install_make_image_wrap (const char *dir, const char 
>*prefix,
>   grub_install_make_image_wrap_file (dir, prefix, fp, outname,
>                                    memdisk_path, config_path,
>                                    mkimage_target, note);
>-  grub_util_file_sync (fp);
>+  if (grub_util_file_sync (fp) < 0)
>+    grub_util_error (_("cannot sync `%s': %s"), outname, strerror (errno));
>   fclose (fp);
> }
> 
>diff --git a/util/grub-mkimage.c b/util/grub-mkimage.c
>index 98d24cc06..912564e36 100644
>--- a/util/grub-mkimage.c
>+++ b/util/grub-mkimage.c
>@@ -311,8 +311,12 @@ main (int argc, char *argv[])
>                              arguments.image_target, arguments.note,
>                              arguments.comp, arguments.dtb);
> 
>-  grub_util_file_sync  (fp);
>-  fclose (fp);
>+  if (grub_util_file_sync (fp) < 0)
>+    grub_util_error (_("cannot sync `%s': %s"), arguments.output ? : "stdout",
>+                   strerror (errno));
>+  if (fclose (fp) == EOF)
>+    grub_util_error (_("cannot close `%s': %s"), arguments.output ? : 
>"stdout",
>+                   strerror (errno));
> 
>   for (i = 0; i < arguments.nmodules; i++)
>     free (arguments.modules[i]);
>diff --git a/util/setup.c b/util/setup.c
>index 9c1e1b7da..8954dd479 100644
>--- a/util/setup.c
>+++ b/util/setup.c
>@@ -728,8 +728,10 @@ unable_to_embed:
>       != GRUB_DISK_SECTOR_SIZE * 2)
>     grub_util_error (_("cannot write to `%s': %s"),
>                    core_path, strerror (errno));
>-  grub_util_fd_sync (fp);
>-  grub_util_fd_close (fp);
>+  if (grub_util_fd_sync (fp) < 0)
>+    grub_util_error (_("cannot sync `%s': %s"), core_path, strerror (errno));
>+  if (grub_util_fd_close (fp) < 0)
>+    grub_util_error (_("cannot close `%s': %s"), core_path, strerror (errno));
>   grub_util_biosdisk_flush (root_dev->disk);
> 
>   grub_disk_cache_invalidate_all ();
>-- 
>2.20.1
>
>_______________________________________________
>Grub-devel mailing list
>address@hidden
>https://lists.gnu.org/mailman/listinfo/grub-devel
>
-- 
Steve McIntyre, Cambridge, UK.                                address@hidden
  Mature Sporty Personal
  More Innovation More Adult
  A Man in Dandism
  Powered Midship Specialty