* [PATCH] util: Detect more I/O errors
[not found] <155065044692.1778.10181739878482866355.reportbug@frontier>
@ 2019-02-27 9:10 ` Colin Watson
2019-02-27 10:14 ` Steve McIntyre
` (3 more replies)
0 siblings, 4 replies; 6+ messages in thread
From: Colin Watson @ 2019-02-27 9:10 UTC (permalink / raw)
To: grub-devel; +Cc: George Caswell, 922741-forwarded
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(-)
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
^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [PATCH] util: Detect more I/O errors
2019-02-27 9:10 ` [PATCH] util: Detect more I/O errors Colin Watson
@ 2019-02-27 10:14 ` Steve McIntyre
2019-02-28 9:33 ` Colin Watson
` (2 subsequent siblings)
3 siblings, 0 replies; 6+ messages in thread
From: Steve McIntyre @ 2019-02-27 10:14 UTC (permalink / raw)
To: The development of GNU GRUB; +Cc: 922741-forwarded, George Caswell
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 <93sam@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. steve@einval.com
Mature Sporty Personal
More Innovation More Adult
A Man in Dandism
Powered Midship Specialty
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] util: Detect more I/O errors
2019-02-27 9:10 ` [PATCH] util: Detect more I/O errors Colin Watson
2019-02-27 10:14 ` Steve McIntyre
@ 2019-02-28 9:33 ` Colin Watson
2019-02-28 16:32 ` Elliott, Robert (Persistent Memory)
2019-03-01 11:35 ` Daniel Kiper
3 siblings, 0 replies; 6+ messages in thread
From: Colin Watson @ 2019-02-28 9:33 UTC (permalink / raw)
To: grub-devel
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.
Oh, I forgot to include it in the commit message, but this is:
Signed-off-by: Colin Watson <cjwatson@ubuntu.com>
--
Colin Watson [cjwatson@ubuntu.com]
^ permalink raw reply [flat|nested] 6+ messages in thread
* RE: [PATCH] util: Detect more I/O errors
2019-02-27 9:10 ` [PATCH] util: Detect more I/O errors Colin Watson
2019-02-27 10:14 ` Steve McIntyre
2019-02-28 9:33 ` Colin Watson
@ 2019-02-28 16:32 ` Elliott, Robert (Persistent Memory)
2019-03-01 9:53 ` Colin Watson
2019-03-01 11:35 ` Daniel Kiper
3 siblings, 1 reply; 6+ messages in thread
From: Elliott, Robert (Persistent Memory) @ 2019-02-28 16:32 UTC (permalink / raw)
To: The development of GNU GRUB; +Cc: 922741-forwarded, George Caswell
> -----Original Message-----
> From: Grub-devel <grub-devel-bounces+elliott=hpe.com@gnu.org> On
> Behalf Of Colin Watson
> Sent: Wednesday, February 27, 2019 3:10 AM
> Subject: [PATCH] util: Detect more I/O errors
>
...
> -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));
> }
Since that's just returning -1 for an error (both for fflush and fsync),
not errno which contains the reason for the error...
> 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));
callers like this will interpret the -1 as EPERM, which isn't the
true reason.
---
Robert Elliott, HPE Persistent Memory
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] util: Detect more I/O errors
2019-02-28 16:32 ` Elliott, Robert (Persistent Memory)
@ 2019-03-01 9:53 ` Colin Watson
0 siblings, 0 replies; 6+ messages in thread
From: Colin Watson @ 2019-03-01 9:53 UTC (permalink / raw)
To: The development of GNU GRUB; +Cc: 922741-forwarded, George Caswell
On Thu, Feb 28, 2019 at 04:32:44PM +0000, Elliott, Robert (Persistent Memory) wrote:
> > -----Original Message-----
> > From: Grub-devel <grub-devel-bounces+elliott=hpe.com@gnu.org> On
> > Behalf Of Colin Watson
> ...
> > -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));
> > }
>
> Since that's just returning -1 for an error (both for fflush and fsync),
> not errno which contains the reason for the error...
>
> > 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));
>
> callers like this will interpret the -1 as EPERM, which isn't the
> true reason.
No, this is a mistaken analysis. The code you're quoting passes errno
to strerror when rendering the error message, *not* the return code of
grub_util_file_sync. It is therefore not possible for it to
misinterpret -1 as EPERM. The same holds for all similar call sites in
my patch. (In any case, the value of EPERM is typically 1, not -1; you
may be thinking of the kernel practice of returning -errno, which is not
common in userspace code.)
The pattern I've followed for grub_util_file_sync and other similar
functions is the commonplace one used by many C library functions: it
either returns 0, or it returns some other value (in this case -1) and
sets errno to indicate the error. It is of course necessary to ensure
that it only returns -1 when something has already set errno, but I
believe that's the case throughout my patch.
--
Colin Watson [cjwatson@ubuntu.com]
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] util: Detect more I/O errors
2019-02-27 9:10 ` [PATCH] util: Detect more I/O errors Colin Watson
` (2 preceding siblings ...)
2019-02-28 16:32 ` Elliott, Robert (Persistent Memory)
@ 2019-03-01 11:35 ` Daniel Kiper
3 siblings, 0 replies; 6+ messages in thread
From: Daniel Kiper @ 2019-03-01 11:35 UTC (permalink / raw)
To: Colin Watson; +Cc: grub-devel, 922741-forwarded, George Caswell
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.
Reviewed-by: Daniel Kiper <daniel.kiper@oracle.com>
Daniel
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2019-03-01 11:37 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
[not found] <155065044692.1778.10181739878482866355.reportbug@frontier>
2019-02-27 9:10 ` [PATCH] util: Detect more I/O errors Colin Watson
2019-02-27 10:14 ` Steve McIntyre
2019-02-28 9:33 ` Colin Watson
2019-02-28 16:32 ` Elliott, Robert (Persistent Memory)
2019-03-01 9:53 ` Colin Watson
2019-03-01 11:35 ` Daniel Kiper
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.