[Date Prev][Date Next] [Thread Prev][Thread Next] [Date Index] [Thread Index]

Bug#990531: marked as done (unblock: grub2/2.04-19)

Your message dated Fri, 2 Jul 2021 21:33:19 +0200
with message-id <1ee40330-d516-d837-9c86-0cd0b776bb11@debian.org>
and subject line Re: Bug#990531: unblock: grub2/2.04-19
has caused the Debian Bug report #990531,
regarding unblock: grub2/2.04-19
to be marked as done.

This means that you claim that the problem has been dealt with.
If this is not the case it is now your responsibility to reopen the
Bug report if necessary, and/or fix the problem forthwith.

(NB: If you are a system administrator and have no idea what this
message is talking about, this may indicate a serious mail system
misconfiguration somewhere. Please contact owner@bugs.debian.org
immediately.)


-- 
990531: https://bugs.debian.org/cgi-bin/bugreport.cgi?bug=990531
Debian Bug Tracking System
Contact owner@bugs.debian.org with problems
--- Begin Message --- 
Package: release.debian.org
Severity: normal
User: release.debian.org@packages.debian.org
Usertags: unblock

Please unblock grub2 2.04-19.  This fixes unbootability problems after
certain kinds of grub-install failure, which I think constituted an
important-severity bug at the very least.  I did this by resyncing the
grub-install backup/restore patches with what actually landed upstream;
we'd previously had early versions of these that hadn't been entirely
bug-fixed.

I know that there may be more RC things to be fixed in grub2, but we
might as well get this in.

Here are the substantive bits of the debdiff, with some git-dpm noise
(in patch headers) filtered out.

diff -Nru grub2-2.04/debian/changelog grub2-2.04/debian/changelog
--- grub2-2.04/debian/changelog	2021-04-25 16:20:17.000000000 +0100
+++ grub2-2.04/debian/changelog	2021-06-19 13:04:38.000000000 +0100
@@ -1,3 +1,11 @@
+grub2 (2.04-19) unstable; urgency=medium
+
+  * Resync grub-install backup and restore patches from upstream, fixing
+    problems that left the system unbootable after certain kinds of failure
+    (closes: #983435).
+
+ -- Colin Watson <cjwatson@debian.org>  Sat, 19 Jun 2021 13:04:38 +0100
+
 grub2 (2.04-18) unstable; urgency=medium
 
   [ Steve McIntyre ]
diff -Nru grub2-2.04/debian/patches/grub-install-backup-and-restore.patch grub2-2.04/debian/patches/grub-install-backup-and-restore.patch
--- grub2-2.04/debian/patches/grub-install-backup-and-restore.patch	2021-04-25 16:20:17.000000000 +0100
+++ grub2-2.04/debian/patches/grub-install-backup-and-restore.patch	2021-06-19 13:04:38.000000000 +0100
@@ -1,34 +1,58 @@
-From b7fc63d1e961a65bcf548594579f014caa122841 Mon Sep 17 00:00:00 2001
+From f59453da4fe9cd4323893daa31d0e42f06f58c4a Mon Sep 17 00:00:00 2001
 From: Dimitri John Ledkov <xnox@ubuntu.com>
-Date: Wed, 19 Aug 2020 01:49:09 +0100
+Date: Tue, 1 Jun 2021 11:35:36 +0100
 Subject: grub-install: Add backup and restore
 
-Refactor clean_grub_dir to create a backup of all the files, instead
+Refactor clean_grub_dir() to create a backup of all the files, instead
 of just irrevocably removing them as the first action. If available,
-register on_exit handle to restore the backup if any errors occur, or
-remove the backup if everything was successful. If on_exit is not
-available, the backup remains on disk for manual recovery.
+register atexit() handler to restore the backup if errors occur before
+point of no return, or remove the backup if everything was successful.
+If atexit() is not available, the backup remains on disk for manual
+recovery.
+
+Some platforms defined a point of no return, i.e. after modules & core
+images were updated. Failures from any commands after that stage are
+ignored, and backup is cleaned up. For example, on EFI platforms update
+is not reverted when efibootmgr fails.
+
+Extra care is taken to ensure atexit() handler is only invoked by the
+parent process and not any children forks. Some older GRUB codebases
+can invoke parent atexit() hooks from forks, which can mess up the
+backup.
 
 This allows safer upgrades of MBR & modules, such that
 modules/images/fonts/translations are consistent with MBR in case of
 errors. For example accidental grub-install /dev/non-existent-disk
 currently clobbers and upgrades modules in /boot/grub, despite not
-actually updating any MBR. This increases peak disk-usage slightly, by
-requiring temporarily twice the disk space to complete grub-install.
+actually updating any MBR.
 
-Also add modinfo.sh to the cleanup/backup/restore codepath, to ensure
-it is also cleaned / backed up / restored.
+This patch only handles backup and restore of files copied to /boot/grub.
+This patch does not perform backup (or restoration) of MBR itself or
+blocklists. Thus when installing i386-pc platform, corruption may still
+occur with MBR and blocklists which will not be attempted to be
+automatically recovered.
+
+Also add modinfo.sh and *.efi to the cleanup/backup/restore code path,
+to ensure it is also cleaned, backed up and restored.
 
 Signed-off-by: Dimitri John Ledkov <xnox@ubuntu.com>
+Reviewed-by: Daniel Kiper <daniel.kiper@oracle.com>
+
+Last-Update: 2021-06-14
 
 Patch-Name: grub-install-backup-and-restore.patch
 ---
- configure.ac               |   2 +-
- util/grub-install-common.c | 105 +++++++++++++++++++++++++++++++------
- 2 files changed, 91 insertions(+), 16 deletions(-)
+ configure.ac                |   2 +-
+ include/grub/util/install.h |  21 +++++
+ util/grub-install-common.c  | 156 ++++++++++++++++++++++++++++++++----
+ util/grub-install.c         |  36 +++++++--
+ util/grub-mknetdir.c        |   3 +
+ util/grub-mkrescue.c        |   3 +
+ util/grub-mkstandalone.c    |   2 +
+ 7 files changed, 200 insertions(+), 23 deletions(-)
 
 diff --git a/configure.ac b/configure.ac
-index 851f61546..fcebe364f 100644
+index 851f61546..a2d4a5d6b 100644
 --- a/configure.ac
 +++ b/configure.ac
 @@ -410,7 +410,7 @@ else
@@ -36,15 +60,45 @@
  
  # Check for functions and headers.
 -AC_CHECK_FUNCS(posix_memalign memalign getextmntent)
-+AC_CHECK_FUNCS(posix_memalign memalign getextmntent on_exit)
++AC_CHECK_FUNCS(posix_memalign memalign getextmntent atexit)
  AC_CHECK_HEADERS(sys/param.h sys/mount.h sys/mnttab.h limits.h)
  
  # glibc 2.25 still includes sys/sysmacros.h in sys/types.h but emits deprecation
+diff --git a/include/grub/util/install.h b/include/grub/util/install.h
+index a521f1663..34ecdf317 100644
+--- a/include/grub/util/install.h
++++ b/include/grub/util/install.h
+@@ -273,4 +273,25 @@ grub_util_get_target_name (const struct grub_install_image_target_desc *t);
+ extern char *grub_install_copy_buffer;
+ #define GRUB_INSTALL_COPY_BUFFER_SIZE 1048576
+ 
++/*
++ * grub-install-common tries to make backups of modules & auxiliary files,
++ * and restore the backup upon failure to install core.img. There are
++ * platforms with additional actions after modules & core got installed
++ * in place. It is a point of no return, as core.img cannot be reverted
++ * from this point onwards, and new modules should be kept installed.
++ * Before performing these additional actions call grub_set_install_backup_ponr()
++ * to set the grub_install_backup_ponr flag. This way failure to perform
++ * subsequent actions will not result in reverting new modules to the
++ * old ones, e.g. in case efivars updates fails.
++ */
++#ifdef HAVE_ATEXIT
++extern void
++grub_set_install_backup_ponr (void);
++#else
++static inline void
++grub_set_install_backup_ponr (void)
++{
++}
++#endif
++
+ #endif
 diff --git a/util/grub-install-common.c b/util/grub-install-common.c
-index 447504d3f..61f9075bc 100644
+index 447504d3f..5e8808f6f 100644
 --- a/util/grub-install-common.c
 +++ b/util/grub-install-common.c
-@@ -185,38 +185,113 @@ grub_install_mkdir_p (const char *dst)
+@@ -185,38 +185,164 @@ grub_install_mkdir_p (const char *dst)
    free (t);
  }
  
@@ -53,30 +107,42 @@
 +{
 +  char *bsuffix = grub_util_path_concat_ext (1, b, ext);
 +  int r = strcmp (a, bsuffix);
++
 +  free (bsuffix);
 +  return r;
 +}
 +
 +enum clean_grub_dir_mode
 +{
-+  CLEAN = 0,
-+  CLEAN_BACKUP = 1,
-+  CREATE_BACKUP = 2,
-+  RESTORE_BACKUP = 3,
++  CLEAN_NEW,
++  CLEAN_BACKUP,
++  CREATE_BACKUP,
++  RESTORE_BACKUP
 +};
 +
++#ifdef HAVE_ATEXIT
++static size_t backup_dirs_size = 0;
++static char **backup_dirs = NULL;
++static pid_t backup_process = 0;
++static int grub_install_backup_ponr = 0;
++
++void
++grub_set_install_backup_ponr (void)
++{
++  grub_install_backup_ponr = 1;
++}
++#endif
++
  static void
 -clean_grub_dir (const char *di)
 +clean_grub_dir_real (const char *di, enum clean_grub_dir_mode mode)
  {
    grub_util_fd_dir_t d;
    grub_util_fd_dirent_t de;
-+  char suffix[2] = "";
++  const char *suffix = "";
 +
 +  if ((mode == CLEAN_BACKUP) || (mode == RESTORE_BACKUP))
-+    {
-+      strcpy (suffix, "-");
-+    }
++    suffix = "~";
  
    d = grub_util_fd_opendir (di);
    if (!d)
@@ -99,9 +165,11 @@
 -	   && strcmp (de->d_name, "menu.lst") != 0)
 -	  || strcmp (de->d_name, "efiemu32.o") == 0
 -	  || strcmp (de->d_name, "efiemu64.o") == 0)
++
 +      if ((ext && (strcmp_ext (ext, ".mod", suffix) == 0
 +		   || strcmp_ext (ext, ".lst", suffix) == 0
 +		   || strcmp_ext (ext, ".img", suffix) == 0
++		   || strcmp_ext (ext, ".efi", suffix) == 0
 +		   || strcmp_ext (ext, ".mo", suffix) == 0)
 +	   && strcmp_ext (de->d_name, "menu.lst", suffix) != 0)
 +	  || strcmp_ext (de->d_name, "modinfo.sh", suffix) == 0
@@ -117,7 +185,8 @@
 +
 +	  if (mode == CREATE_BACKUP)
 +	    {
-+	      char *dstf = grub_util_path_concat_ext (2, di, de->d_name, "-");
++	      char *dstf = grub_util_path_concat_ext (2, di, de->d_name, "~");
++
 +	      if (grub_util_rename (srcf, dstf) < 0)
 +		grub_util_error (_("cannot backup `%s': %s"), srcf,
 +				 grub_util_fd_strerror ());
@@ -126,7 +195,8 @@
 +	  else if (mode == RESTORE_BACKUP)
 +	    {
 +	      char *dstf = grub_util_path_concat (2, di, de->d_name);
-+	      dstf[strlen (dstf) - 1] = 0;
++
++	      dstf[strlen (dstf) - 1] = '\0';
 +	      if (grub_util_rename (srcf, dstf) < 0)
 +		grub_util_error (_("cannot restore `%s': %s"), dstf,
 +				 grub_util_fd_strerror ());
@@ -144,32 +214,190 @@
    grub_util_fd_closedir (d);
  }
  
++#ifdef HAVE_ATEXIT
 +static void
-+restore_backup_on_exit (int status, void *arg)
++restore_backup_atexit (void)
 +{
-+  if (status == 0)
++  size_t i;
++
++  /*
++   * Some child inherited atexit() handler, did not clear it, and called it.
++   * Thus skip clean or restore logic.
++   */
++  if (backup_process != getpid ())
++    return;
++
++  for (i = 0; i < backup_dirs_size; i++)
 +    {
-+      clean_grub_dir_real ((char *) arg, CLEAN_BACKUP);
++      /*
++       * If past point of no return simply clean the backups. Otherwise
++       * cleanup newly installed files, and restore the backups.
++       */
++      if (grub_install_backup_ponr)
++	clean_grub_dir_real (backup_dirs[i], CLEAN_BACKUP);
++      else
++	{
++	  clean_grub_dir_real (backup_dirs[i], CLEAN_NEW);
++	  clean_grub_dir_real (backup_dirs[i], RESTORE_BACKUP);
++	}
++      free (backup_dirs[i]);
 +    }
-+  else
++
++  backup_dirs_size = 0;
++
++  free (backup_dirs);
++}
++
++static void
++append_to_backup_dirs (const char *dir)
++{
++  backup_dirs = xrealloc (backup_dirs, sizeof (char *) * (backup_dirs_size + 1));
++  backup_dirs[backup_dirs_size] = xstrdup (dir);
++  backup_dirs_size++;
++  if (!backup_process)
 +    {
-+      clean_grub_dir_real ((char *) arg, CLEAN);
-+      clean_grub_dir_real ((char *) arg, RESTORE_BACKUP);
++      atexit (restore_backup_atexit);
++      backup_process = getpid ();
 +    }
-+  free (arg);
-+  arg = NULL;
 +}
++#else
++static void
++append_to_backup_dirs (const char *dir __attribute__ ((unused)))
++{
++}
++#endif
 +
 +static void
 +clean_grub_dir (const char *di)
 +{
 +  clean_grub_dir_real (di, CLEAN_BACKUP);
 +  clean_grub_dir_real (di, CREATE_BACKUP);
-+#if defined(HAVE_ON_EXIT)
-+  on_exit (restore_backup_on_exit, strdup (di));
-+#endif
++  append_to_backup_dirs (di);
 +}
 +
  struct install_list
  {
    int is_default;
+diff --git a/util/grub-install.c b/util/grub-install.c
+index bd6d8dbb3..bf34ff554 100644
+--- a/util/grub-install.c
++++ b/util/grub-install.c
+@@ -1869,9 +1869,13 @@ main (int argc, char *argv[])
+ 			
+ 	/*  Now perform the installation.  */
+ 	if (install_bootsector)
+-	  grub_util_bios_setup (platdir, "boot.img", "core.img",
+-				install_drive, force,
+-				fs_probe, allow_floppy, add_rs_codes);
++	  {
++	    grub_util_bios_setup (platdir, "boot.img", "core.img",
++				  install_drive, force,
++				  fs_probe, allow_floppy, add_rs_codes);
++
++	    grub_set_install_backup_ponr ();
++	  }
+ 
+ 	/* If vestiges of GRUB Legacy still exist, tell the Debian packaging
+ 	   that they can ignore them.  */
+@@ -1908,10 +1912,14 @@ main (int argc, char *argv[])
+ 			
+ 	/*  Now perform the installation.  */
+ 	if (install_bootsector)
+-	  grub_util_sparc_setup (platdir, "boot.img", "core.img",
+-				 install_drive, force,
+-				 fs_probe, allow_floppy,
+-				 0 /* unused */ );
++	  {
++	    grub_util_sparc_setup (platdir, "boot.img", "core.img",
++				   install_drive, force,
++				   fs_probe, allow_floppy,
++				   0 /* unused */ );
++
++	    grub_set_install_backup_ponr ();
++	  }
+ 	break;
+       }
+ 
+@@ -1938,6 +1946,8 @@ main (int argc, char *argv[])
+ 	  grub_elf = grub_util_path_concat (2, core_services, "grub.elf");
+ 	  grub_install_copy_file (imgfile, grub_elf, 1);
+ 
++	  grub_set_install_backup_ponr ();
++
+ 	  f = grub_util_fopen (mach_kernel, "a+");
+ 	  if (!f)
+ 	    grub_util_error (_("Can't create file: %s"), strerror (errno));
+@@ -2038,6 +2048,8 @@ main (int argc, char *argv[])
+ 	  boot_efi = grub_util_path_concat (2, core_services, "boot.efi");
+ 	  grub_install_copy_file (imgfile, boot_efi, 1);
+ 
++	  grub_set_install_backup_ponr ();
++
+ 	  f = grub_util_fopen (mach_kernel, "r+");
+ 	  if (!f)
+ 	    grub_util_error (_("Can't create file: %s"), strerror (errno));
+@@ -2198,6 +2210,8 @@ main (int argc, char *argv[])
+ 	      also_install_removable (imgfile, base_efidir, removable_file, 1);
+ 	  }
+ 
++	grub_set_install_backup_ponr ();
++
+ 	free (removable_file);
+ 	free (dst);
+       }
+@@ -2270,6 +2284,14 @@ main (int argc, char *argv[])
+       break;
+     }
+ 
++  /*
++   * Either there are no platform specific code, or it didn't raise
++   * ponr. Raise it here, because usually this is already past point
++   * of no return. If we leave this flag false, at exit all the modules
++   * will be removed from the prefix which would be very confusing.
++   */
++  grub_set_install_backup_ponr ();
++
+   fprintf (stderr, "%s\n", _("Installation finished. No error reported."));
+ 
+   /* Free resources.  */
+diff --git a/util/grub-mknetdir.c b/util/grub-mknetdir.c
+index 602574d52..a2461cda1 100644
+--- a/util/grub-mknetdir.c
++++ b/util/grub-mknetdir.c
+@@ -159,6 +159,9 @@ process_input_dir (const char *input_dir, enum grub_install_plat platform)
+   grub_install_make_image_wrap (input_dir, prefix, output,
+ 				0, load_cfg,
+ 				targets[platform].mkimage_target, 0);
++
++  grub_set_install_backup_ponr ();
++
+   grub_install_pop_module ();
+ 
+   /* TRANSLATORS: First %s is replaced by platform name. Second one by filename.  */
+diff --git a/util/grub-mkrescue.c b/util/grub-mkrescue.c
+index cb972f120..fcb35726c 100644
+--- a/util/grub-mkrescue.c
++++ b/util/grub-mkrescue.c
+@@ -530,6 +530,9 @@ main (int argc, char *argv[])
+ 			       boot_grub, plat);
+       source_dirs[plat] = xstrdup (grub_install_source_directory);
+     }
++
++  grub_set_install_backup_ponr ();
++
+   if (system_area == SYS_AREA_AUTO || grub_install_source_directory)
+     {
+       if (source_dirs[GRUB_INSTALL_PLATFORM_I386_PC]
+diff --git a/util/grub-mkstandalone.c b/util/grub-mkstandalone.c
+index edf309717..5f50a3b84 100644
+--- a/util/grub-mkstandalone.c
++++ b/util/grub-mkstandalone.c
+@@ -318,6 +318,8 @@ main (int argc, char *argv[])
+   grub_install_copy_files (grub_install_source_directory,
+ 			   boot_grub, plat);
+ 
++  grub_set_install_backup_ponr ();
++
+   char *memdisk_img = grub_util_make_temporary_file ();
+ 
+   memdisk = grub_util_fopen (memdisk_img, "wb");
diff -Nru grub2-2.04/debian/patches/osdep-exec-avoid-atexit-when-child-exits.patch grub2-2.04/debian/patches/osdep-exec-avoid-atexit-when-child-exits.patch
--- grub2-2.04/debian/patches/osdep-exec-avoid-atexit-when-child-exits.patch	1970-01-01 01:00:00.000000000 +0100
+++ grub2-2.04/debian/patches/osdep-exec-avoid-atexit-when-child-exits.patch	2021-06-19 13:04:38.000000000 +0100
@@ -0,0 +1,48 @@
+From 82fbd3f55cf0379ee6aa7661462324b30ac2fdd5 Mon Sep 17 00:00:00 2001
+From: Dimitri John Ledkov <xnox@ubuntu.com>
+Date: Thu, 29 Apr 2021 12:34:34 +0100
+Subject: osdep/unix/exec: Avoid atexit() handlers when child execvp() fails
+
+The functions grub_util_exec_pipe() and grub_util_exec_pipe_stderr()
+currently call execvp(). If the call fails for any reason, the child
+currently calls exit(127). This in turn executes the parents
+atexit() handlers from the forked child, and then the same handlers
+are called again from parent. This is usually not desired, and can
+lead to deadlocks, and undesired behavior. So, change the exit() calls
+to _exit() calls to avoid calling atexit() handlers from child.
+
+Fixes: e75cf4a58 (unix exec: avoid atexit handlers when child exits)
+
+Signed-off-by: Dimitri John Ledkov <xnox@ubuntu.com>
+Reviewed-by: Daniel Kiper <daniel.kiper@oracle.com>
+
+Bug-Debian: https://bugs.debian.org/983435
+Last-Update: 2021-06-14
+
+Patch-Name: osdep-exec-avoid-atexit-when-child-exits.patch
+---
+ grub-core/osdep/unix/exec.c | 4 ++--
+ 1 file changed, 2 insertions(+), 2 deletions(-)
+
+diff --git a/grub-core/osdep/unix/exec.c b/grub-core/osdep/unix/exec.c
+index db3259f65..e8db9202f 100644
+--- a/grub-core/osdep/unix/exec.c
++++ b/grub-core/osdep/unix/exec.c
+@@ -188,7 +188,7 @@ grub_util_exec_pipe (const char *const *argv, int *fd)
+       close (pipe_fd[1]);
+ 
+       execvp ((char *) argv[0], (char **) argv);
+-      exit (127);
++      _exit (127);
+     }
+   else
+     {
+@@ -234,7 +234,7 @@ grub_util_exec_pipe_stderr (const char *const *argv, int *fd)
+       close (pipe_fd[1]);
+ 
+       execvp ((char *) argv[0], (char **) argv);
+-      exit (127);
++      _exit (127);
+     }
+   else
+     {
diff -Nru grub2-2.04/debian/patches/series grub2-2.04/debian/patches/series
--- grub2-2.04/debian/patches/series	2021-04-25 16:20:17.000000000 +0100
+++ grub2-2.04/debian/patches/series	2021-06-19 13:04:38.000000000 +0100
@@ -94,6 +94,7 @@
 bootp-alloc.patch
 unix-config-overflow.patch
 deviceiter-overflow.patch
+osdep-exec-avoid-atexit-when-child-exits.patch
 grub-install-backup-and-restore.patch
 tftp-roll-over-block-counter.patch
 mdraid1x-linux-gcc-10.patch

unblock grub2/2.04-19

Thanks,

-- 
Colin Watson (he/him)                              [cjwatson@debian.org]

--- End Message ---
--- Begin Message --- 
Hi,

On 02-07-2021 05:38, Cyril Brulebois wrote:
> Paul Gevers <elbrus@debian.org> (2021-07-01):
>> I had a look at this the other day and if my memory recalls correctly,
>> I'm fine with it. But as there's a block-udeb, we need an ACK from kibi
>> (in CC via boot).
> 
> grub2 and its signed packages are welcome, thanks.

Unblocked.

Paul

Attachment: OpenPGP_signature
Description: OpenPGP digital signature


--- End Message ---
Reply to: