Merge pull request ostreedev#3385 from cgwalters/log-xattr-conflict

core: Fix bare-user xattr canonicalization

src/libostree/ostree-core-private.h

@@ -90,6 +90,7 @@ void _ostree_gfileinfo_to_stbuf (GFileInfo *file_info, struct stat *out_stbuf);
 gboolean _ostree_gfileinfo_equal (GFileInfo *a, GFileInfo *b);
 gboolean _ostree_stbuf_equal (struct stat *stbuf_a, struct stat *stbuf_b);
 GFileInfo *_ostree_mode_uidgid_to_gfileinfo (mode_t mode, uid_t uid, gid_t gid);
+GVariant *_ostree_canonicalize_xattrs (GVariant *xattrs);
 gboolean _ostree_validate_structureof_xattrs (GVariant *xattrs, GError **error);
 
 static inline void

src/libostree/ostree-core.c

@@ -43,7 +43,6 @@ G_STATIC_ASSERT (OSTREE_REPO_MODE_BARE_USER_ONLY == 3);
 G_STATIC_ASSERT (OSTREE_REPO_MODE_BARE_SPLIT_XATTRS == 4);
 
 static GBytes *variant_to_lenprefixed_buffer (GVariant *variant);
-static GVariant *canonicalize_xattrs (GVariant *xattrs);
 
 #define ALIGN_VALUE(this, boundary) \
   ((((unsigned long)(this)) + (((unsigned long)(boundary)) - 1)) \
@@ -317,8 +316,8 @@ compare_xattrs (const void *a_pp, const void *b_pp)
 }
 
 // Sort xattrs by name
-static GVariant *
-canonicalize_xattrs (GVariant *xattrs)
+GVariant *
+_ostree_canonicalize_xattrs (GVariant *xattrs)
 {
   // We always need to provide data, so NULL is canonicalized to the empty array
   if (xattrs == NULL)
@@ -364,7 +363,7 @@ _ostree_file_header_new (GFileInfo *file_info, GVariant *xattrs)
     symlink_target = "";
 
   // We always sort the xattrs now to ensure everything is in normal/canonical form.
-  g_autoptr (GVariant) tmp_xattrs = canonicalize_xattrs (xattrs);
+  g_autoptr (GVariant) tmp_xattrs = _ostree_canonicalize_xattrs (xattrs);
 
   g_autoptr (GVariant) ret
       = g_variant_new ("(uuuus@a(ayay))", GUINT32_TO_BE (uid), GUINT32_TO_BE (gid),
@@ -1153,7 +1152,7 @@ ostree_create_directory_metadata (GFileInfo *dir_info, GVariant *xattrs)
   GVariant *ret_metadata = NULL;
 
   // We always sort the xattrs now to ensure everything is in normal/canonical form.
-  g_autoptr (GVariant) tmp_xattrs = canonicalize_xattrs (xattrs);
+  g_autoptr (GVariant) tmp_xattrs = _ostree_canonicalize_xattrs (xattrs);
 
   ret_metadata = g_variant_new (
       "(uuu@a(ayay))", GUINT32_TO_BE (g_file_info_get_attribute_uint32 (dir_info, "unix::uid")),
@@ -2342,7 +2341,8 @@ _ostree_validate_structureof_xattrs (GVariant *xattrs, GError **error)
           if (cmp == 0)
             return glnx_throw (error, "Duplicate xattr name, index=%d", i);
           else if (cmp > 0)
-            return glnx_throw (error, "Incorrectly sorted xattr name, index=%d", i);
+            return glnx_throw (error, "Incorrectly sorted xattr name (prev=%s, cur=%s), index=%d",
+                               previous, name, i);
         }
       previous = name;
       i++;

src/libostree/ostree-repo-commit.c

@@ -139,9 +139,12 @@ gboolean
 _ostree_write_bareuser_metadata (int fd, guint32 uid, guint32 gid, guint32 mode, GVariant *xattrs,
                                  GError **error)
 {
-  if (xattrs != NULL && !_ostree_validate_structureof_xattrs (xattrs, error))
-    return FALSE;
-  g_autoptr (GVariant) filemeta = create_file_metadata (uid, gid, mode, xattrs);
+  GLNX_AUTO_PREFIX_ERROR ("Writing bareuser metadata", error);
+  // Like we do elsewhere, ensure xattrs are in canonical form. We don't strictly need
+  // this because we don't actually provide this data directly as input to a checksum today,
+  // but it's good hygeine to do so.
+  g_autoptr (GVariant) tmp_xattrs = _ostree_canonicalize_xattrs (xattrs);
+  g_autoptr (GVariant) filemeta = create_file_metadata (uid, gid, mode, tmp_xattrs);
 
   if (TEMP_FAILURE_RETRY (fsetxattr (fd, "user.ostreemeta", (char *)g_variant_get_data (filemeta),
                                      g_variant_get_size (filemeta), 0))

src/libostree/ostree-repo.c

@@ -4098,7 +4098,10 @@ _ostree_repo_load_file_bare (OstreeRepo *self, const char *checksum, int *out_fd
 
       g_autoptr (GVariant) metadata = g_variant_ref_sink (
           g_variant_new_from_bytes (OSTREE_FILEMETA_GVARIANT_FORMAT, bytes, FALSE));
-      ret_xattrs = filemeta_to_stat (&stbuf, metadata);
+      g_autoptr (GVariant) read_xattrs = filemeta_to_stat (&stbuf, metadata);
+      // Old versions of ostree may have written these xattrs in non-canonical form.
+      // In order to aid comparisons, let's canonicalize here.
+      ret_xattrs = _ostree_canonicalize_xattrs (read_xattrs);
       if (S_ISLNK (stbuf.st_mode))
         {
           if (out_symlink)

tests/basic-test.sh

@@ -19,7 +19,7 @@
 
 set -euo pipefail
 
-echo "1..$((90 + ${extra_basic_tests:-0}))"
+echo "1..$((91 + ${extra_basic_tests:-0}))"
 
 CHECKOUT_U_ARG=""
 CHECKOUT_H_ARGS="-H"
@@ -443,6 +443,24 @@ if ! skip_one_without_user_xattrs; then
     echo "ok local clone checkout"
 fi
 
+if test -z "${OSTREE_NO_XATTRS:-}"; then
+    cd ${test_tmpdir}
+    ${CMD_PREFIX} ostree --repo=repo checkout ${CHECKOUT_U_ARG} test2 test2-checkout
+    touch test2-checkout/testxattrs
+    # Intentionally heavily interspersed (not sorted) 
+    for n in a z q e f n c r i a b x t k y; do
+        setfattr -n user.$n -v x test2-checkout/testxattrs
+    done
+    cat repo/config
+    $OSTREE commit ${COMMIT_ARGS} -b test2 --tree=dir=test2-checkout --consume
+    $OSTREE ls -X test2 /testxattrs > out.txt
+    assert_file_has_content_literal out.txt "(b'user.a', [0x78])"
+    assert_file_has_content_literal out.txt "(b'user.z', [0x78])"
+    echo "ok sorted xattrs"
+else
+    echo "ok # SKIP no xattrs"
+fi
+
 $OSTREE checkout -U test2 checkout-user-test2
 echo "ok user checkout"