Hi @HastD. Thanks for your PR.

I'm waiting for a ostreedev member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

Let's try to push this through soon

Copilot reviews have been closed without comments, why use a temporary variable at all ?

@champtar The called function has signature GVariant *_ostree_filter_selinux_xattr (GVariant *xattrs), which means it could modify the input xattrs in place. Generally speaking, I consider it to be clearer coding style to avoid assigning to a variable in the same statement as an expression that could modify that variable. However, if you think it'd be better in this case, I can change it to remove the use of the temporary variable.

Nop that's ok with me, I'll leave it to someone else to merge if anyone has stronger opinions on this

In create_file_copy_from_input_at and checkout_tree_at_recurse, both xattrs and modified_xattrs are declared with g_autoptr, but later on, xattrs is simply assigned to be equal to modified_xattrs, meaning the automatic cleanup is a double-free.

Not...quite. I believe there's a bug here, but my initial take is this patch is changing a double free to a memory leak. This merits deeper investigation.

Let's look at this code pattern:

static gboolean
create_file_copy_from_input_at (..., GVariant *xattrs, ...)
{
  g_autoptr (GVariant) modified_xattrs = NULL;

  if (condition)
    {
      modified_xattrs = _ostree_filter_selinux_xattr (xattrs);
      xattrs = modified_xattrs;
    }

A really important thing to understand is that in the C programming language, the assignment of xattrs only happens inside that function. The calling function does not see the change.
Just try running this for example:

#include <stdio.h>

void foo(char *p) {
  p = "foo";
}

int main() {
    const char *p = "bar";
    foo(p);
    printf("%s\n", p);
    return 0;
}

It will print "bar".

This pattern of "constant pointer and owned pointer" is used in many other places in the code. It is safe if used correctly - and again I could totally believe there's a bug here.

(Insert obligatory "this wouldn't happen in Rust" thing and I am so looking forward to migrating most logic here to bootc/composefs which are all in Rust)

It's going to take me a bit but let me investigate before we merge please (cc @champtar ).

(Or of course anyone else can feel free to do so, but this just needs a careful analysis)

What seems most likely here to me is us handling floating references with GVariant incorrectly; it wouldn't be the first time.

The thing is floating references were created to make reference counting easier in C but that was before we popularized g_autoptr. The two things clash badly.

In a nutshell what can easily go wrong is when one passes a floating reference into a function which consumes it. The value of xattrs being passed in should not be floating...but if it is I think that would cause this double free.

I tried to do some manual auditing here but yeah, high level of tedium, running an AI agent on this to do an audit, we'll see.

TL;DR can you retest with reverting just the first hunk?

I think the sole change we need is

diff --git i/src/libostree/ostree-repo-checkout.c w/src/libostree/ostree-repo-checkout.c
index ccddb488..1feb9be6 100644
--- i/src/libostree/ostree-repo-checkout.c
+++ w/src/libostree/ostree-repo-checkout.c
@@ -997,7 +997,6 @@ checkout_tree_at_recurse (OstreeRepo *self, OstreeRepoCheckoutAtOptions *options
   g_autoptr (GVariant) dirtree = NULL;
   g_autoptr (GVariant) dirmeta = NULL;
   g_autoptr (GVariant) xattrs = NULL;
-  g_autoptr (GVariant) modified_xattrs = NULL;
 
   if (!ostree_repo_load_variant (self, OSTREE_OBJECT_TYPE_DIR_TREE, dirtree_checksum, &dirtree,
                                  error))
@@ -1055,8 +1054,8 @@ checkout_tree_at_recurse (OstreeRepo *self, OstreeRepoCheckoutAtOptions *options
     if (sepolicy_enabled && _ostree_sepolicy_host_enabled (options->sepolicy))
       {
         /* We'll set the xattr via setfscreatecon(), so don't do it via generic xattrs below. */
-        modified_xattrs = _ostree_filter_selinux_xattr (xattrs);
-        xattrs = modified_xattrs;
+        g_autoptr(GVariant) old_xattrs = g_steal_pointer (&xattrs);
+        xattrs = _ostree_filter_selinux_xattr (old_xattrs);
 
         if (!_ostree_sepolicy_preparefscreatecon (&fscreatecon, options->sepolicy,
                                                   state->selabel_path_buf->str, mode, error))

I just retested the revised version (which I've now pushed to this PR) in the same VM, and it does indeed fix the bug. Thanks @cgwalters for catching that memory leak in the earlier version.

/override ci/prow/fcos-e2e

@cgwalters: Overrode contexts on behalf of cgwalters: ci/prow/fcos-e2e