local-store: make extended attribute handling more robust

* Move the extended attribute deletion after the hardlink sanity check. We
  shouldn't be removing extended attributes on random files.
* Make the entity owner-writable before attempting to remove extended
  attributes, since this operation usually requires write access on the file,
  and we shouldn't fail xattr deletion on a file that has been made unwritable
  by the builder or a previous canonicalisation pass.

Fixes: https://git.lix.systems/lix-project/lix/issues/507
Change-Id: I7e6ccb71649185764cd5210f4a4794ee174afea6

1 files changed, 16 insertions, 14 deletions

diff --git a/src/libstore/local-store.cc b/src/libstore/local-store.cc
index 49991e38a..2da8dd2a6 100644
--- a/src/libstore/local-store.cc
+++ b/src/libstore/local-store.cc
@@ -664,6 +664,20 @@ static void canonicalisePathMetaData_(
     if (!(S_ISREG(st.st_mode) || S_ISDIR(st.st_mode) || S_ISLNK(st.st_mode)))
         throw Error("file '%1%' has an unsupported type", path);
 
+    /* Fail if the file is not owned by the build user.  This prevents
+       us from messing up the ownership/permissions of files
+       hard-linked into the output (e.g. "ln /etc/shadow $out/foo").
+       However, ignore files that we chown'ed ourselves previously to
+       ensure that we don't fail on hard links within the same build
+       (i.e. "touch $out/foo; ln $out/foo $out/bar"). */
+    if (uidRange && (st.st_uid < uidRange->first || st.st_uid > uidRange->second)) {
+        if (S_ISDIR(st.st_mode) || !inodesSeen.count(Inode(st.st_dev, st.st_ino)))
+            throw BuildError("invalid ownership on file '%1%'", path);
+        mode_t mode = st.st_mode & ~S_IFMT;
+        assert(S_ISLNK(st.st_mode) || (st.st_uid == geteuid() && (mode == 0444 || mode == 0555) && st.st_mtime == mtimeStore));
+        return;
+    }
+
 #if __linux__
     /* Remove extended attributes / ACLs. */
     ssize_t eaSize = llistxattr(path.c_str(), nullptr, 0);
@@ -677,6 +691,8 @@ static void canonicalisePathMetaData_(
         if ((eaSize = llistxattr(path.c_str(), eaBuf.data(), eaBuf.size())) < 0)
             throw SysError("querying extended attributes of '%s'", path);
 
+        if (S_ISREG(st.st_mode) || S_ISDIR(st.st_mode))
+            chmod(path.c_str(), st.st_mode | S_IWUSR);
         for (auto & eaName: tokenizeString<Strings>(std::string(eaBuf.data(), eaSize), std::string("\000", 1))) {
             if (settings.ignoredAcls.get().count(eaName)) continue;
             if (lremovexattr(path.c_str(), eaName.c_str()) == -1)
@@ -685,20 +701,6 @@ static void canonicalisePathMetaData_(
      }
 #endif
 
-    /* Fail if the file is not owned by the build user.  This prevents
-       us from messing up the ownership/permissions of files
-       hard-linked into the output (e.g. "ln /etc/shadow $out/foo").
-       However, ignore files that we chown'ed ourselves previously to
-       ensure that we don't fail on hard links within the same build
-       (i.e. "touch $out/foo; ln $out/foo $out/bar"). */
-    if (uidRange && (st.st_uid < uidRange->first || st.st_uid > uidRange->second)) {
-        if (S_ISDIR(st.st_mode) || !inodesSeen.count(Inode(st.st_dev, st.st_ino)))
-            throw BuildError("invalid ownership on file '%1%'", path);
-        mode_t mode = st.st_mode & ~S_IFMT;
-        assert(S_ISLNK(st.st_mode) || (st.st_uid == geteuid() && (mode == 0444 || mode == 0555) && st.st_mtime == mtimeStore));
-        return;
-    }
-
     inodesSeen.insert(Inode(st.st_dev, st.st_ino));
 
     canonicaliseTimestampAndPermissions(path, st);