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 <93...@debian.org> > >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 >Grub-devel@gnu.org >https://lists.gnu.org/mailman/listinfo/grub-devel > -- Steve McIntyre, Cambridge, UK. st...@einval.com Mature Sporty Personal More Innovation More Adult A Man in Dandism Powered Midship Specialty _______________________________________________ Grub-devel mailing list Grub-devel@gnu.org https://lists.gnu.org/mailman/listinfo/grub-devel