PR #1803: actually cache permissions modified by chmod_min()

lib/build_cache.py

@@ -263,13 +263,12 @@ class File_Metadata:
    #   st ............ Stat object for the file. Absent after un-pickling.
 
    def __init__(self, image_root, path):
+      # Note: Constructor not called during unpickle.
       self.image_root = image_root
       self.path = path
       self.path_abs = image_root // path
       self.st = self.path_abs.stat(False)
-      # Note: Constructor not called during unpickle.
-      for attr in ("atime_ns", "mtime_ns", "mode", "size"):
-         setattr(self, attr, getattr(self.st, "st_" + attr))
+      self.stat_cache_update()
       self.children = dict()
       self.dont_restore = False
       self.hardlink_to = None
@@ -351,9 +350,11 @@ def git_prepare(class_, image_root, large_file_thresh,
          # skip Git stuff at image root
          fm.dont_restore = True
          return fm
-      # Ensure minimum permissions. Some tools like to make files with mode
-      # 000, because root ignores the permissions bits.
-      fm.path_abs.chmod_min(fm.st)
+      # Ensure minimum permissions. Some tools like to make files without
+      # necessary owner permissions, because root ignores the permissions bits
+      # (CAP_DAC_OVERRIDE). See e.g. #1765.
+      fm.st = fm.path_abs.chmod_min(fm.st)
+      fm.stat_cache_update()
       # Validate file type and recurse if needed. (Don’t use os.walk() because
       # it’s iterative, and our algorithm is better expressed recursively.)
       if   (   stat.S_ISREG(fm.mode)
@@ -548,6 +549,10 @@ def pickle(self):
       (self.image_root // PICKLE_PATH) \
          .file_write(pickle.dumps(self, protocol=4))
 
+   def stat_cache_update(self):
+      for attr in ("atime_ns", "mtime_ns", "mode", "size"):
+         setattr(self, attr, getattr(self.st, "st_" + attr))
+
    def str_for_log(self):
       # Truncate reported time to seconds.
       fmt = "%Y-%m-%d.%H:%M:%S"

lib/filesystem.py

@@ -662,26 +662,46 @@ def chdir(self):
       ch.ossafe("can’t chdir(2): %s" % self, os.chdir, self)
       return self.__class__(old)
 
-   def chmod_min(self, st=None):
+   def chmod_min(self, st_old=None):
       """Set my permissions to at least 0o700 for directories and 0o400
-         otherwise. If given, st is a stat object for self, to avoid another
-         stat(2) call. Return the new file mode (permissions and file type).
+         otherwise.
 
          For symlinks, do nothing, because we don’t want to follow symlinks
          and follow_symlinks=False (or os.lchmod) is not supported on some
          (all?) Linux. (Also, symlink permissions are ignored on Linux, so it
-         doesn’t matter anyway.)"""
-      if (st is None):
-         st = self.stat(False)
+         doesn’t matter anyway.)
+
+         If given, st_old is a stat_result object for self, to avoid another
+         stat(2) call. In this case, also return the resulting stat_result
+         object, which is st itself if nothing was modified, or a new
+         stat_result object if the mode was changed."""
+      st = self.stat(False) if not st_old else st_old
       if (stat.S_ISLNK(st.st_mode)):
-         return st.st_mode
+         return st_old
       perms_old = stat.S_IMODE(st.st_mode)
       perms_new = perms_old | (0o700 if stat.S_ISDIR(st.st_mode) else 0o400)
       if (perms_new != perms_old):
          ch.VERBOSE("fixing permissions: %s: %03o -> %03o"
                  % (self, perms_old, perms_new))
          ch.ossafe("can’t chmod: %s" % self, os.chmod, self, perms_new)
-      return (st.st_mode | perms_new)
+      if (st_old):
+         # stat_result is a deeply weird object (a “structsec” rather than a
+         # named tuple), including multiple values for the same field when
+         # accessed by index vs. name. I did figure out how to create a
+         # modified copy, which is the commented code below, but it seems too
+         # brittle and scary, so just re-stat(2) the modified metadata.
+         #
+         #   st_list = list(st_old)
+         #   st_dict = { k:getattr(a, k) for k in dir(a) if k[:3] == "st_" }
+         #   st_list[0] |= perms_new
+         #   st_dict["st_mode"] |= perms_new
+         #   st_new = os.stat_result(st_list, st_dict)
+         #   assert (st_new[0] == st_new.st_mode)
+         #   return st_new
+         if (perms_new == perms_old):
+            return st_old
+         else:
+            return self.stat(False)
 
    def copy(self, dst):
       """Copy file myself to dst, including metadata, overwriting dst if it

test/build/50_ch-image.bats

@@ -944,3 +944,17 @@ EOF
     [[ $output = *'1* FROM alpine:3.16'* ]]
     [[ $output = *'2. RUN.S true'* ]]
 }
+
+
+@test "dnf --installroot" {  # issue #1765
+    export CH_IMAGE_STORAGE=$BATS_TMPDIR/dnf_installroot
+    df=$BATS_TMPDIR/dnf_installroot.df
+
+    cat > "$df" <<EOF
+FROM almalinux:8
+RUN dnf install -y --releasever=/ --installroot=/foo filesystem
+EOF
+
+    ch-image build -f "$df" "$BATS_TMPDIR"
+    ch-image reset
+}