flatpak-dir: Clean up temp deploy dir on failure of flatpak_dir_deploy()

[pwithnall]

This already happens for installs due to the cleanup path in
flatpak_dir_deploy_install(), but it doesn’t happen for other calls to
flatpak_dir_deploy(). Notably, during updates of already installed
apps.

Specifically, this means that if an app update is cancelled due to being
blocked by a parental controls policy, the temp deploy dir for that app
(such as
~/.local/share/flatpak/app/com.corp.App/x86_64/stable/.somehex-XXXXXX)
will be leaked. It will never be automatically cleaned up, as it’s not
in /var/tmp either.

Fix that by using glnx_mkdtempat() to create a scoped temporary
directory.

Signed-off-by: Philip Withnall pwithnall@endlessos.org

[pwithnall]

You can fairly easily test this by applying the following patch to flatpak, and then running flatpak update --user some.app.which.needs.Updating with and without this PR applied. Without the PR applied, a temp directory will be leaked in ~/.local/share/flatpak/app/some.app.which.needs.Updating/x86_64/stable/ for each flatpak update run. With the PR applied, it won’t.

Testing patch:

From 3f9d1969fd7bf1aae7fa2f9aa776b831c418d20b Mon Sep 17 00:00:00 2001
From: Philip Withnall <pwithnall@endlessos.org>
Date: Wed, 19 Oct 2022 15:50:16 +0100
Subject: [PATCH] WIP Force failure

Signed-off-by: Philip Withnall <pwithnall@endlessos.org>
---
 common/flatpak-dir.c | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/common/flatpak-dir.c b/common/flatpak-dir.c
index ca64902c..a3a9495e 100644
--- a/common/flatpak-dir.c
+++ b/common/flatpak-dir.c
@@ -8892,6 +8892,12 @@ flatpak_dir_deploy (FlatpakDir          *self,
   if (!flatpak_dir_check_parental_controls (self, flatpak_decomposed_get_ref (ref), deploy_data, cancellable, error))
     return FALSE;
 
+  if (strstr (flatpak_decomposed_get_ref (ref), "app"))
+    {
+      g_set_error_literal (error, G_IO_ERROR, G_IO_ERROR_FAILED, "Wibble");
+      return FALSE;
+    }
+
   deploy_data_file = g_file_get_child (checkoutdir, "deploy");
   if (!flatpak_bytes_save (deploy_data_file, deploy_data, cancellable, error))
     return FALSE;
-- 
2.37.3

[wjt]

CI failed with (possibly among other errors):

common/flatpak-dir.c:8547:20: error: unused variable 'tmp_dir_path_template' [-Werror,-Wunused-variable]
  g_autofree char *tmp_dir_path_template = NULL;
                   ^
common/flatpak-dir.c:8546:20: error: unused variable 'tmp_dir_template' [-Werror,-Wunused-variable]
  g_autoptr(GFile) tmp_dir_template = NULL;

[pwithnall]

oops, fixed that, thanks

[GeorgesStavracas]

I don't think I truly understand this part of the code, but left a comment nonetheless

[GeorgesStavracas]

LGTM

[pwithnall]

The tests failed due to timeouts in test-oci-registry@user.wrap and test-oci-registry@system.wrap on valgrind. Is that expected? These tests succeeded without valgrind, so I suspect it’s just a slowness issue.

[pwithnall]

I’ve rebased on latest main

[pwithnall]

Once this has landed on main, would you like me to do a backport to 1.12 and 1.10? Backporting it to 1.10 would be useful for Endless OS because we ship that on EOS 4, but only if you’re still happy to do 1.10 releases. Otherwise I’m happy to backport it as a distro patch.

I guess a backport to 1.12 would be less controversial.

Happy to do whichever you’d prefer.

In any case, I would likely backport the disk space leak commit, but not the commit to use more FD-relative operations, as that isn’t strictly fixing a bug.

[smcv]

1.14.x is the current stable branch. 1.12.x and 1.10.x are documented as still being supported, but the older they get, the more the balance tilts towards "security fixes only", and whether we backport a bug fix depends on the balance of bug impact vs. regression risk.

1.10.x is actually more likely to keep getting releases than 1.12.x, because it's what is in Debian 11, and I'm responsible for that until mid 2024. I should probably assess what we do and don't apply to 1.10.x and do a bugfix point release for Debian 11 at some point - if you're maintaining a version derived from 1.10.x for EOS then we can probably compare notes.

I would likely backport the disk space leak commit, but not the commit to use more FD-relative operations, as that isn’t strictly fixing a bug

That sounds good to me.

[smcv]

The tests failed due to timeouts in test-oci-registry@user.wrap and test-oci-registry@system.wrap on valgrind

Unfortunately, it's normal for the "Run tests in valgrind" stage to time out - you're lucky it was only individual tests timing out. At some point we should disable it, or replace it with something quicker, or something, so that we can rely on "CI failed" genuinely being a merge blocker.

[pwithnall]

I’ll prepare backports for 1.14, 1.12 and 1.10, then you can decide which you’d like to merge. It’ll be simple enough, since I’ve got the testing commit to hand.

I should probably assess what we do and don't apply to 1.10.x and do a bugfix point release for Debian 11 at some point - if you're maintaining a version derived from 1.10.x for EOS then we can probably compare notes.

The list of commits we currently have downstream on top of 1.10.7 is here: https://github.com/endlessm/flatpak/commits/eos4.0, and the packaging changes are https://github.com/endlessm/flatpak/commits/debian-eos4.0, if that helps.

[pwithnall]

I’ll prepare backports for 1.14, 1.12 and 1.10, then you can decide which you’d like to merge. It’ll be simple enough, since I’ve got the testing commit to hand.

⇒ #5157, #5158, #5159

[pwithnall]

I’ve opened #5164 with a change to automatically clean up old leaked tmpdirs too. It’s not currently in the backports, but I can add it to them if you think that’s worthwhile (I’m a bit hesitant about backporting an rm -rf)