Merge ~xnox/grub:ubuntu-backup-and-restore-v2 into ~ubuntu-core-dev/grub/+git/ubuntu:ubuntu

[Dimitri John Ledkov]

This merge request addresses reliability and safety of applying grub-pc upgrades.
When performed non-interactively, postinst instead of showing critical prompts may simply exit successfully indicating successful package configure even though MBR update may have failed, and modules in /boot/grub might no longer match the MBR core image leading to failure to boot.
Separately, I have submitted a patch upstream to make grub-install more resilient. Instead of irrevocably removing grub modules prior to updating MBR, I now made it create a backup and register on_exit handle. If everything is successful the on_exit handle removes the backup. If critical errors have occurred the backup is restored. This prevents mismatch of grub modules and MBR in case of attempts to call grub-install on non-existing devices, or calling on a device that refuses writes to MBR (as I have seen in one particular public cloud, it felt as if the underlying disk storage was refusing writes to mbr).
I have tested above using apparmor profile that prohibited writes to /dev/*. And ensured that backups/rollback/cleanup all operate correctly using Debian cloud image as a base VM image.

[Dimitri John Ledkov (xnox)]

I'm sorry about git-dpm noise w.r.t. how many characters are used for short hash. I don't know what I need to do to have it the same as previous uploads.

[Steve Langasek (vorlon)]

LGTM. Were you still going to do something with the git dpm hashes (AIUI by adding the upstream remote so that git will recalculate)?

[Steve Langasek (vorlon)]

review: Approve

[Dimitri John Ledkov (xnox)]

`git config --global core.abbrev 10`

worked!

diff --git a/configure.ac b/configure.ac
index 1819188..6a88b9b 100644
--- a/configure.ac
+++ b/configure.ac
@@ -420,7 +420,7 @@ else
 fi
 
 # Check for functions and headers.
-AC_CHECK_FUNCS(posix_memalign memalign getextmntent)
+AC_CHECK_FUNCS(posix_memalign memalign getextmntent on_exit)
 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/debian/.git-dpm b/debian/.git-dpm
index ed27285..f278adb 100644
--- a/debian/.git-dpm
+++ b/debian/.git-dpm
@@ -1,6 +1,6 @@
 # see git-dpm(1) from git-dpm package
-398371c71cd52b6c48fa1d888903bd8a85682ec0
-398371c71cd52b6c48fa1d888903bd8a85682ec0
+0843387b9bedf43212e3148c0c9b6d013565c87e
+0843387b9bedf43212e3148c0c9b6d013565c87e
 578bb115fbd47e1c464696f1f8d6183e5443975d
 578bb115fbd47e1c464696f1f8d6183e5443975d
 grub2_2.04.orig.tar.xz
diff --git a/debian/changelog b/debian/changelog
index f7c0ce2..5fa8da9 100644
--- a/debian/changelog
+++ b/debian/changelog
@@ -1,3 +1,15 @@
+grub2 (2.04-1ubuntu29) UNRELEASED; urgency=medium
+
+  * grub-install: cherry-pick patch from grub-devel to make grub-install
+    fault tolerant. Create backup of files in /boot/grub, and restore them
+    on failure to complete grub-install. LP: #1891680
+  * postinst.in: do not exit successfully when failing to show critical
+    grub-pc/install_devices_failed and grub-pc/install_devices_empty
+    prompts in non-interactive mode. This enables surfacing upgrade errors
+    to the users and/or automation. LP: #1891680
+
+ -- Dimitri John Ledkov <[e-mail]>  Fri, 28 Aug 2020 15:15:45 +0100
+
 grub2 (2.04-1ubuntu28) groovy; urgency=medium
 
   * Ensure that grub-multi-install can always find templates (LP: #1879948)
diff --git a/debian/patches/grub-install-backup-and-restore.patch b/debian/patches/grub-install-backup-and-restore.patch
new file mode 100644
index 0000000..b947b44
--- /dev/null
+++ b/debian/patches/grub-install-backup-and-restore.patch
@@ -0,0 +1,175 @@
+From 0843387b9bedf43212e3148c0c9b6d013565c87e Mon Sep 17 00:00:00 2001
+From: Dimitri John Ledkov <[e-mail]>
+Date: Wed, 19 Aug 2020 01:49:09 +0100
+Subject: grub-install: Add backup and restore
+
+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.
+
+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.
+
+Also add modinfo.sh to the cleanup/backup/restore codepath, to ensure
+it is also cleaned / backed up / restored.
+
+Signed-off-by: Dimitri John Ledkov <[e-mail]>
+
+Patch-Name: grub-install-backup-and-restore.patch
+---
+ configure.ac               |   2 +-
+ util/grub-install-common.c | 105 +++++++++++++++++++++++++++++++------
+ 2 files changed, 91 insertions(+), 16 deletions(-)
+
+diff --git a/configure.ac b/configure.ac
+index 1819188f9f..6a88b9b0c0 100644
+--- a/configure.ac
++++ b/configure.ac
+@@ -420,7 +420,7 @@ else
+ fi
+ 
+ # Check for functions and headers.
+-AC_CHECK_FUNCS(posix_memalign memalign getextmntent)
++AC_CHECK_FUNCS(posix_memalign memalign getextmntent on_exit)
+ 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/util/grub-install-common.c b/util/grub-install-common.c
+index 447504d3f4..a883b6daef 100644
+--- a/util/grub-install-common.c
++++ b/util/grub-install-common.c
+@@ -185,38 +185,113 @@ grub_install_mkdir_p (const char *dst)
+   free (t);
+ }
+ 
++static int
++strcmp_ext (const char *a, const char *b, const char *ext)
++{
++  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,
++};
++
+ 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] = "";
++
++  if ((mode == CLEAN_BACKUP) || (mode == RESTORE_BACKUP))
++    {
++      strcpy (suffix, "-");
++    }
+ 
+   d = grub_util_fd_opendir (di);
+   if (!d)
+-    grub_util_error (_("cannot open directory `%s': %s"),
+-		     di, grub_util_fd_strerror ());
++    {
++      if (mode == CLEAN_BACKUP)
++	return;
++      grub_util_error (_("cannot open directory `%s': %s"),
++		       di, grub_util_fd_strerror ());
++    }
+ 
+   while ((de = grub_util_fd_readdir (d)))
+     {
+       const char *ext = strrchr (de->d_name, '.');
+-      if ((ext && (strcmp (ext, ".mod") == 0
+-		   || strcmp (ext, ".lst") == 0
+-		   || strcmp (ext, ".img") == 0
+-		   || strcmp (ext, ".mo") == 0)
+-	   && 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, ".mo", suffix) == 0)
++	   && strcmp_ext (de->d_name, "menu.lst", suffix) != 0)
++	  || strcmp_ext (de->d_name, "modinfo.sh", suffix) == 0
++	  || strcmp_ext (de->d_name, "efiemu32.o", suffix) == 0
++	  || strcmp_ext (de->d_name, "efiemu64.o", suffix) == 0)
+ 	{
+-	  char *x = grub_util_path_concat (2, di, de->d_name);
+-	  if (grub_util_unlink (x) < 0)
+-	    grub_util_error (_("cannot delete `%s': %s"), x,
+-			     grub_util_fd_strerror ());
+-	  free (x);
++	  char *srcf = grub_util_path_concat (2, di, de->d_name);
++
++	  if (mode == CREATE_BACKUP)
++	    {
++	      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 ());
++	      free (dstf);
++	    }
++	  else if (mode == RESTORE_BACKUP)
++	    {
++	      char *dstf = grub_util_path_concat_ext (2, di, de->d_name);
++	      dstf[strlen (dstf) - 1] = 0;
++	      if (grub_util_rename (srcf, dstf) < 0)
++		grub_util_error (_("cannot restore `%s': %s"), dstf,
++				 grub_util_fd_strerror ());
++	      free (dstf);
++	    }
++	  else
++	    {
++	      if (grub_util_unlink (srcf) < 0)
++		grub_util_error (_("cannot delete `%s': %s"), srcf,
++				 grub_util_fd_strerror ());
++	    }
++	  free (srcf);
+ 	}
+     }
+   grub_util_fd_closedir (d);
+ }
+ 
++static void
++restore_backup_on_exit (int status, void *arg)
++{
++  if (status == 0)
++    {
++      clean_grub_dir_real ((char *) arg, CLEAN_BACKUP);
++    }
++  else
++    {
++      clean_grub_dir_real ((char *) arg, CLEAN);
++      clean_grub_dir_real ((char *) arg, RESTORE_BACKUP);
++    }
++  free (arg);
++  arg = NULL;
++}
++
++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
++}
++
+ struct install_list
+ {
+   int is_default;
diff --git a/debian/patches/series b/debian/patches/series
index 4f8471c..0cbd443 100644
--- a/debian/patches/series
+++ b/debian/patches/series
@@ -106,3 +106,4 @@ ubuntu-speed-zsys-history.patch
 ubuntu-flavour-order.patch
 ubuntu-dont-verify-loopback-images.patch
 ubuntu-recovery-dis_ucode_ldr.patch
+grub-install-backup-and-restore.patch
diff --git a/debian/postinst.in b/debian/postinst.in
index 3da6939..e11c451 100644
--- a/debian/postinst.in
+++ b/debian/postinst.in
@@ -662,7 +662,7 @@ case "$1" in
                     continue
                   fi
                 else
-                  break # noninteractive
+                  exit 1 # noninteractive
                 fi
               fi
             fi
@@ -685,7 +685,7 @@ case "$1" in
                   db_fset grub-pc/install_devices_empty seen false
                 fi
               else
-                break # noninteractive
+                exit 1 # noninteractive
               fi
             else
               break
diff --git a/util/grub-install-common.c b/util/grub-install-common.c
index 447504d..a883b6d 100644
--- a/util/grub-install-common.c
+++ b/util/grub-install-common.c
@@ -185,38 +185,113 @@ grub_install_mkdir_p (const char *dst)
   free (t);
 }
 
+static int
+strcmp_ext (const char *a, const char *b, const char *ext)
+{
+  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,
+};
+
 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] = "";
+
+  if ((mode == CLEAN_BACKUP) || (mode == RESTORE_BACKUP))
+    {
+      strcpy (suffix, "-");
+    }
 
   d = grub_util_fd_opendir (di);
   if (!d)
-    grub_util_error (_("cannot open directory `%s': %s"),
-		     di, grub_util_fd_strerror ());
+    {
+      if (mode == CLEAN_BACKUP)
+	return;
+      grub_util_error (_("cannot open directory `%s': %s"),
+		       di, grub_util_fd_strerror ());
+    }
 
   while ((de = grub_util_fd_readdir (d)))
     {
       const char *ext = strrchr (de->d_name, '.');
-      if ((ext && (strcmp (ext, ".mod") == 0
-		   || strcmp (ext, ".lst") == 0
-		   || strcmp (ext, ".img") == 0
-		   || strcmp (ext, ".mo") == 0)
-	   && 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, ".mo", suffix) == 0)
+	   && strcmp_ext (de->d_name, "menu.lst", suffix) != 0)
+	  || strcmp_ext (de->d_name, "modinfo.sh", suffix) == 0
+	  || strcmp_ext (de->d_name, "efiemu32.o", suffix) == 0
+	  || strcmp_ext (de->d_name, "efiemu64.o", suffix) == 0)
 	{
-	  char *x = grub_util_path_concat (2, di, de->d_name);
-	  if (grub_util_unlink (x) < 0)
-	    grub_util_error (_("cannot delete `%s': %s"), x,
-			     grub_util_fd_strerror ());
-	  free (x);
+	  char *srcf = grub_util_path_concat (2, di, de->d_name);
+
+	  if (mode == CREATE_BACKUP)
+	    {
+	      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 ());
+	      free (dstf);
+	    }
+	  else if (mode == RESTORE_BACKUP)
+	    {
+	      char *dstf = grub_util_path_concat_ext (2, di, de->d_name);
+	      dstf[strlen (dstf) - 1] = 0;
+	      if (grub_util_rename (srcf, dstf) < 0)
+		grub_util_error (_("cannot restore `%s': %s"), dstf,
+				 grub_util_fd_strerror ());
+	      free (dstf);
+	    }
+	  else
+	    {
+	      if (grub_util_unlink (srcf) < 0)
+		grub_util_error (_("cannot delete `%s': %s"), srcf,
+				 grub_util_fd_strerror ());
+	    }
+	  free (srcf);
 	}
     }
   grub_util_fd_closedir (d);
 }
 
+static void
+restore_backup_on_exit (int status, void *arg)
+{
+  if (status == 0)
+    {
+      clean_grub_dir_real ((char *) arg, CLEAN_BACKUP);
+    }
+  else
+    {
+      clean_grub_dir_real ((char *) arg, CLEAN);
+      clean_grub_dir_real ((char *) arg, RESTORE_BACKUP);
+    }
+  free (arg);
+  arg = NULL;
+}
+
+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
+}
+
 struct install_list
 {
   int is_default;