Skip to content

Commit

Permalink
Merge pull request #3346 from cgwalters/commit-label-ordering
Browse files Browse the repository at this point in the history 
core: Always sort incoming xattrs
  • Loading branch information
@cgwalters
cgwalters authored Dec 3, 2024
2 parents 111a45f + 1858d3d commit 74efebd
Show file tree
Hide file tree
Showing 2 changed files with 132 additions and 16 deletions.
72 changes: 62 additions & 10 deletions src/libostree/ostree-core.c
View file
Original file line number Diff line number Diff line change
Expand Up @@ -43,6 +43,7 @@ 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)) \
Expand Down Expand Up @@ -302,6 +303,47 @@ ostree_validate_collection_id (const char *collection_id, GError **error)
return TRUE;
}

static int
compare_xattrs (const void *a_pp, const void *b_pp)
{
GVariant *a = *(GVariant **)a_pp;
GVariant *b = *(GVariant **)b_pp;
const char *name_a;
const char *name_b;
g_variant_get (a, "(^&ay@ay)", &name_a, NULL);
g_variant_get (b, "(^&ay@ay)", &name_b, NULL);

return strcmp (name_a, name_b);
}

// Sort xattrs by name
static GVariant *
canonicalize_xattrs (GVariant *xattrs)
{
// We always need to provide data, so NULL is canonicalized to the empty array
if (xattrs == NULL)
return g_variant_ref_sink (g_variant_new_array (G_VARIANT_TYPE ("(ayay)"), NULL, 0));

g_autoptr (GPtrArray) xattr_array
= g_ptr_array_new_with_free_func ((GDestroyNotify)g_variant_unref);

const guint n = g_variant_n_children (xattrs);
for (guint i = 0; i < n; i++)
{
GVariant *child = g_variant_get_child_value (xattrs, i);
g_ptr_array_add (xattr_array, child);
}

g_ptr_array_sort (xattr_array, compare_xattrs);

GVariantBuilder builder;
g_variant_builder_init (&builder, G_VARIANT_TYPE ("a(ayay)"));
for (guint i = 0; i < xattr_array->len; i++)
g_variant_builder_add_value (&builder, xattr_array->pdata[i]);

return g_variant_ref_sink (g_variant_builder_end (&builder));
}

/* The file header is part of the "object stream" format
* that's not compressed. It's comprised of uid,gid,mode,
* and possibly symlink targets from @file_info, as well
Expand All @@ -321,13 +363,12 @@ _ostree_file_header_new (GFileInfo *file_info, GVariant *xattrs)
else
symlink_target = "";

g_autoptr (GVariant) tmp_xattrs = NULL;
if (xattrs == NULL)
tmp_xattrs = g_variant_ref_sink (g_variant_new_array (G_VARIANT_TYPE ("(ayay)"), NULL, 0));
// 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) ret
= g_variant_new ("(uuuus@a(ayay))", GUINT32_TO_BE (uid), GUINT32_TO_BE (gid),
GUINT32_TO_BE (mode), 0, symlink_target, xattrs ?: tmp_xattrs);
GUINT32_TO_BE (mode), 0, symlink_target, tmp_xattrs);
return variant_to_lenprefixed_buffer (g_variant_ref_sink (ret));
}

Expand Down Expand Up @@ -1111,11 +1152,13 @@ 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);

ret_metadata = g_variant_new (
"(uuu@a(ayay))", GUINT32_TO_BE (g_file_info_get_attribute_uint32 (dir_info, "unix::uid")),
GUINT32_TO_BE (g_file_info_get_attribute_uint32 (dir_info, "unix::gid")),
GUINT32_TO_BE (g_file_info_get_attribute_uint32 (dir_info, "unix::mode")),
xattrs ? xattrs : g_variant_new_array (G_VARIANT_TYPE ("(ayay)"), NULL, 0));
GUINT32_TO_BE (g_file_info_get_attribute_uint32 (dir_info, "unix::mode")), tmp_xattrs);
g_variant_ref_sink (ret_metadata);

return ret_metadata;
Expand Down Expand Up @@ -2278,21 +2321,30 @@ ostree_validate_structureof_file_mode (guint32 mode, GError **error)
}

/* Currently ostree imposes no restrictions on xattrs on its own;
* they can e.g. be arbitrariliy sized or in number.
* However, we do validate the key is non-empty, as that is known
* to always fail.
* they can e.g. be arbitrariliy sized or in number. The xattrs
* must be sorted by name (without duplicates), and keys cannot be empty.
*/
gboolean
_ostree_validate_structureof_xattrs (GVariant *xattrs, GError **error)
{
const guint n = g_variant_n_children (xattrs);
const char *previous = NULL;
for (guint i = 0; i < n; i++)
{
const guint8 *name;
const char *name;
g_autoptr (GVariant) value = NULL;
g_variant_get_child (xattrs, i, "(^&ay@ay)", &name, &value);
if (!*name)
return glnx_throw (error, "Invalid xattr name (empty or missing NUL) index=%d", i);
if (previous)
{
int cmp = strcmp (previous, name);
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);
}
previous = name;
i++;
}
return TRUE;
Expand Down
76 changes: 70 additions & 6 deletions tests/test-basic-c.c
View file
Original file line number Diff line number Diff line change
Expand Up @@ -130,12 +130,12 @@ test_raw_file_to_archive_stream (gconstpointer data)
}

static gboolean
hi_content_stream_new (GInputStream **out_stream, guint64 *out_length, GError **error)
basic_regfile_content_stream_new (const char *contents, GVariant *xattr, GInputStream **out_stream,
guint64 *out_length, GError **error)
{
static const char hi[] = "hi";
const size_t len = sizeof (hi) - 1;
const size_t len = strlen (contents);
g_autoptr (GMemoryInputStream) hi_memstream
= (GMemoryInputStream *)g_memory_input_stream_new_from_data (hi, len, NULL);
= (GMemoryInputStream *)g_memory_input_stream_new_from_data (contents, len, NULL);
g_autoptr (GFileInfo) finfo = g_file_info_new ();
g_file_info_set_attribute_uint32 (finfo, "standard::type", G_FILE_TYPE_REGULAR);
g_file_info_set_attribute_boolean (finfo, "standard::is-symlink", FALSE);
Expand All @@ -147,6 +147,12 @@ hi_content_stream_new (GInputStream **out_stream, guint64 *out_length, GError **
out_length, NULL, error);
}

static gboolean
hi_content_stream_new (GInputStream **out_stream, guint64 *out_length, GError **error)
{
return basic_regfile_content_stream_new ("hi", NULL, out_stream, out_length, error);
}

static void
test_validate_remotename (void)
{
Expand Down Expand Up @@ -174,6 +180,8 @@ test_object_writes (gconstpointer data)

static const char hi_sha256[]
= "2301b5923720c3edc1f0467addb5c287fd5559e3e0cd1396e7f1edb6b01be9f0";
static const char invalid_hi_sha256[]
= "cafebabecafebabecafebabecafebabecafebabecafebabecafebabecafebabe";

/* Successful content write */
{
Expand All @@ -193,12 +201,68 @@ test_object_writes (gconstpointer data)
hi_content_stream_new (&hi_memstream, &len, &error);
g_assert_no_error (error);
g_autofree guchar *csum = NULL;
static const char invalid_hi_sha256[]
= "cafebabecafebabecafebabecafebabecafebabecafebabecafebabecafebabe";
g_assert (!ostree_repo_write_content (repo, invalid_hi_sha256, hi_memstream, len, &csum, NULL,
&error));
g_assert (error);
g_assert (strstr (error->message, "Corrupted file object"));
g_clear_error (&error);
}

/* Test a basic regfile inline write, no xattrs */
g_assert (ostree_repo_write_regfile_inline (repo, hi_sha256, 0, 0, S_IFREG | 0644, NULL,
(guint8 *)"hi", 2, NULL, &error));
g_assert_no_error (error);

/* Test a basic regfile inline write, erroring on checksum */
g_assert (!ostree_repo_write_regfile_inline (repo, invalid_hi_sha256, 0, 0, S_IFREG | 0644, NULL,
(guint8 *)"hi", 2, NULL, &error));
g_assert (error != NULL);
g_clear_error (&error);

static const char expected_sha256_with_xattrs[]
= "72473a9e8ace75f89f1504137cfb134feb5372bc1d97e04eb5300e24ad836153";

GVariantBuilder builder;
g_variant_builder_init (&builder, G_VARIANT_TYPE ("a(ayay)"));
g_variant_builder_add (&builder, "(^ay^ay)", "security.selinux", "foo_t");
g_variant_builder_add (&builder, "(^ay^ay)", "user.blah", "somedata");
g_autoptr (GVariant) inorder_xattrs = g_variant_ref_sink (g_variant_builder_end (&builder));
g_variant_builder_init (&builder, G_VARIANT_TYPE ("a(ayay)"));
g_variant_builder_add (&builder, "(^ay^ay)", "user.blah", "somedata");
g_variant_builder_add (&builder, "(^ay^ay)", "security.selinux", "foo_t");
g_autoptr (GVariant) unsorted_xattrs = g_variant_ref_sink (g_variant_builder_end (&builder));

/* Now test with xattrs */
g_assert (ostree_repo_write_regfile_inline (repo, expected_sha256_with_xattrs, 0, 0,
S_IFREG | 0644, inorder_xattrs, (guint8 *)"hi", 2,
NULL, &error));
g_assert_no_error (error);

/* And now with a swapped order */
g_assert (ostree_repo_write_regfile_inline (repo, expected_sha256_with_xattrs, 0, 0,
S_IFREG | 0644, unsorted_xattrs, (guint8 *)"hi", 2,
NULL, &error));
g_assert_no_error (error);

/* Tests of directory metadata */
static const char expected_dirmeta_sha256[]
= "f773ab98198d8e46f77be6ffff5fc1920888c0af5794426c3b1461131d509f34";
g_autoptr (GFileInfo) fi = g_file_info_new ();
g_file_info_set_attribute_uint32 (fi, "unix::uid", 0);
g_file_info_set_attribute_uint32 (fi, "unix::gid", 0);
g_file_info_set_attribute_uint32 (fi, "unix::mode", (0755 | S_IFDIR));
{
g_autoptr (GVariant) dirmeta = ostree_create_directory_metadata (fi, inorder_xattrs);
ostree_repo_write_metadata (repo, OSTREE_OBJECT_TYPE_DIR_META, expected_dirmeta_sha256, dirmeta,
NULL, NULL, &error);
g_assert_no_error (error);
}
/* And now with unsorted xattrs */
{
g_autoptr (GVariant) dirmeta = ostree_create_directory_metadata (fi, unsorted_xattrs);
ostree_repo_write_metadata (repo, OSTREE_OBJECT_TYPE_DIR_META, expected_dirmeta_sha256, dirmeta,
NULL, NULL, &error);
g_assert_no_error (error);
}
}

Expand Down

0 comments on commit 74efebd

Please sign in to comment.