Hi!
[ Sorry, was meaning to send this out earlier, but lost track of it. ]
On Tue, 2023-01-31 at 17:26:35 +0100, Lukasz Zemczak wrote:
> Package: dpkg
> Version: 1.21.19
> Tags: patch
> While working on a rather specific project in Ubuntu, we encountered
> an issue with dpkg when trying to handle read-only directories. As we
> had a read-only filesystem mounted on the system, we decided to use
> dpkg's --path-exclude to willingly ignore directories on that
> filesystem, as it is supported. Sadly, we saw that dpkg was still
> erroring out while trying to "unable to clean up mess surrounding
> (...)". After diving into the code, my colleagues noticed that the
> code for checking for interrupted transactions performs a rename()
> function call, but only expects ENOENT and ENOTDIR as 'expected'
> errors. In case of a read-only directory the errno is EROFS.
>
> One way would be to add EROFS to the set of expected errors in this
> case, but Julian Klode made a point that we might, in some cases,
> error out on a read-only filesystem indeed. Instead he proposed to
> simply do a check for the .dpkg-tmp file before trying to perform the
> rename. And this seems to work as expected.
>
> Change is relatively low-risk. We will be pushing this to our Ubuntu
> packages, but also wanted to make sure it ends up upstream! Can you
> take a look at the patch and merge it in if all checks out?
Thanks for the report and the patch! It seems this condition was
missed when previous issues with RO filesystems were handled in the
past.
I've instead gone with the attached patch, which I queued at the time
and follows the same pattern as existing code in the project, which
avoids penalizing the common case.
Thanks,
Guillem
From 0689b5b4a144d9d7c2171a12f6ff84da63a61892 Mon Sep 17 00:00:00 2001
From: Guillem Jover <guil...@debian.org>
Date: Mon, 22 May 2023 23:49:59 +0200
Subject: [PATCH] dpkg: Handle non-existent .dpkg-tmp files on read-only
filesystems
On read-only filesystems trying to rename a directory or a file will
fail with EROFS, even if non-existent. To avoid impacting the common
case, check whether errno is EROFS, then check whether the file exists
and if so restore errno to EROFS to fail on that, otherwise let the
errno from the failure trickle into the next check which will ignore it
if it implies that the file does not exist.
Closes: #1030149
---
src/main/archives.c | 9 +++++++++
1 file changed, 9 insertions(+)
diff --git a/src/main/archives.c b/src/main/archives.c
index 0062600c2..1ecce545a 100644
--- a/src/main/archives.c
+++ b/src/main/archives.c
@@ -762,6 +762,15 @@ tarobject(struct tar_archive *tar, struct tar_entry *ti)
* backup/restore operation and were rudely interrupted.
* So, we see if we have .dpkg-tmp, and if so we restore it. */
if (rename(fnametmpvb.buf,fnamevb.buf)) {
+ /* Trying to remove a directory or a file on a read-only filesystem,
+ * even if non-existent, always returns EROFS. */
+ if (errno == EROFS) {
+ /* If the file does not exist the access function will remap the
+ * EROFS into an ENOENT, otherwise restore EROFS to fail with that. */
+ if (access(fnametmpvb.buf, F_OK) == 0)
+ errno = EROFS;
+ }
+
if (errno != ENOENT && errno != ENOTDIR)
ohshite(_("unable to clean up mess surrounding '%.255s' before "
"installing another version"), ti->name);
--
2.40.1
