Blob Blame History Raw
From 3a5d2ee0f56d049927c4dcdd03e8e275ac03e20d Mon Sep 17 00:00:00 2001
From: Panu Matilainen <pmatilai@redhat.com>
Date: Mon, 8 Feb 2021 10:45:59 +0200
Subject: [PATCH 01/10] Clean up file unpack iteration logic a bit

Handle rpmfiNext() in the while-condition directly to make it more like
similar other constructs elsewhere, adjust for the end of iteration
code after the loop. Also take the file index from rpmfiNext() so
we don't need multiple calls to rpmfiFX() later.
---
 lib/fsm.c | 19 +++++++------------
 1 file changed, 7 insertions(+), 12 deletions(-)

diff --git a/lib/fsm.c b/lib/fsm.c
index 432bcbd90..f71b9bc1e 100644
--- a/lib/fsm.c
+++ b/lib/fsm.c
@@ -865,6 +865,7 @@ int rpmPackageFilesInstall(rpmts ts, rpmte te, rpmfiles files,
     struct stat sb;
     int saveerrno = errno;
     int rc = 0;
+    int fx = -1;
     int nodigest = (rpmtsFlags(ts) & RPMTRANS_FLAG_NOFILEDIGEST) ? 1 : 0;
     int nofcaps = (rpmtsFlags(ts) & RPMTRANS_FLAG_NOCAPS) ? 1 : 0;
     int firsthardlink = -1;
@@ -886,17 +887,8 @@ int rpmPackageFilesInstall(rpmts ts, rpmte te, rpmfiles files,
     /* Detect and create directories not explicitly in package. */
     rc = fsmMkdirs(files, fs, plugins);
 
-    while (!rc) {
-	/* Read next payload header. */
-	rc = rpmfiNext(fi);
-
-	if (rc < 0) {
-	    if (rc == RPMERR_ITER_END)
-		rc = 0;
-	    break;
-	}
-
-	action = rpmfsGetAction(fs, rpmfiFX(fi));
+    while (!rc && (fx = rpmfiNext(fi)) >= 0) {
+	action = rpmfsGetAction(fs, fx);
 	skip = XFA_SKIPPING(action);
 	if (action != FA_TOUCH) {
 	    suffix = S_ISDIR(rpmfiFMode(fi)) ? NULL : tid;
@@ -920,7 +912,7 @@ int rpmPackageFilesInstall(rpmts ts, rpmte te, rpmfiles files,
 	if (rc) {
 	    skip = 1;
 	} else {
-	    setFileState(fs, rpmfiFX(fi));
+	    setFileState(fs, fx);
 	}
 
         if (!skip) {
@@ -1022,6 +1014,9 @@ touch:
 	fpath = _free(fpath);
     }
 
+    if (!rc && fx != RPMERR_ITER_END)
+	rc = fx;
+
     rpmswAdd(rpmtsOp(ts, RPMTS_OP_UNCOMPRESS), fdOp(payload, FDSTAT_READ));
     rpmswAdd(rpmtsOp(ts, RPMTS_OP_DIGEST), fdOp(payload, FDSTAT_DIGEST));
 
-- 
2.34.1


From 3096acbddc57eb2b65c96aad1ed3524ea9d0cead Mon Sep 17 00:00:00 2001
From: Panu Matilainen <pmatilai@redhat.com>
Date: Wed, 10 Feb 2021 08:25:28 +0200
Subject: [PATCH 02/10] Drop unused filename variable

---
 lib/fsm.c | 2 --
 1 file changed, 2 deletions(-)

diff --git a/lib/fsm.c b/lib/fsm.c
index f71b9bc1e..8e6a6c08b 100644
--- a/lib/fsm.c
+++ b/lib/fsm.c
@@ -976,11 +976,9 @@ touch:
 	    /* On FA_TOUCH no hardlinks are created thus this is skipped. */
 	    /* we skip the hard linked file containing the content */
 	    /* write the content to the first used instead */
-	    char *fn = rpmfilesFN(files, firsthardlink);
 	    rc = rpmfiArchiveReadToFilePsm(fi, firstlinkfile, nodigest, psm);
 	    wfd_close(&firstlinkfile);
 	    firsthardlink = -1;
-	    free(fn);
 	}
 
         if (rc) {
-- 
2.34.1


From 9561315538f1fc4a5b207a43ac39805134e7153f Mon Sep 17 00:00:00 2001
From: Panu Matilainen <pmatilai@redhat.com>
Date: Wed, 10 Feb 2021 09:57:17 +0200
Subject: [PATCH 03/10] Don't update path info if rename failed on file commit

---
 lib/fsm.c | 16 +++++++++-------
 1 file changed, 9 insertions(+), 7 deletions(-)

diff --git a/lib/fsm.c b/lib/fsm.c
index 8e6a6c08b..138b55297 100644
--- a/lib/fsm.c
+++ b/lib/fsm.c
@@ -797,14 +797,16 @@ static int fsmCommit(char **path, rpmfi fi, rpmFileAction action, const char *su
 	/* Rename temporary to final file name if needed. */
 	if (dest != *path) {
 	    rc = fsmRename(*path, dest);
-	    if (!rc && nsuffix) {
-		char * opath = fsmFsPath(fi, NULL);
-		rpmlog(RPMLOG_WARNING, _("%s created as %s\n"),
-		       opath, dest);
-		free(opath);
+	    if (!rc) {
+		if (nsuffix) {
+		    char * opath = fsmFsPath(fi, NULL);
+		    rpmlog(RPMLOG_WARNING, _("%s created as %s\n"),
+			   opath, dest);
+		    free(opath);
+		}
+		free(*path);
+		*path = dest;
 	    }
-	    free(*path);
-	    *path = dest;
 	}
     }
 
-- 
2.34.1


From 73d090166a60c57484e4be40e46fcb07f026cbf8 Mon Sep 17 00:00:00 2001
From: Panu Matilainen <pmatilai@redhat.com>
Date: Thu, 27 Aug 2020 10:31:07 +0300
Subject: [PATCH 04/10] Upgrade FA_TOUCH to FA_CREATE if the file went away
 (RhBug:1872141)

When %_minimize_writes is enabled, we determine unchanged files during
fingerprinting and only update their metadata (FA_TOUCH) instead of
always recreating from scratch (FA_CREATE) during install. However
package scriptlets (and administrators) can and will do arbitrary stuff
in the meanwhile, such as rm -f their own files in %pre, hoping to
get a fresh copy of contents no matter what. Or something.
Now, if the file was determined to not need changing by rpm, this will
just fail with chown & friends trying to touch non-existent file.
One can consider this a case of package shooting itself in the foot, but
when a package update fails or succeeds depending on %_minimize_writes this
to me suggests the feature is at fault as much as the package.

Do fsmVerify() on all files to be FA_TOUCH'ed to detect files whose
type changed or were removed since fingerprinting. This still doesn't
ensure correctness if something tampers with the contents in the meanwhile,
(for that we'd need to run the file through the whole machinery again,
checksumming and all) but covers the most glaring cases.
---
 lib/fsm.c                      | 15 ++++++---
 tests/Makefile.am              |  1 +
 tests/data/SPECS/suicidal.spec | 19 ++++++++++++
 tests/rpmi.at                  | 56 ++++++++++++++++++++++++++++++++++
 4 files changed, 87 insertions(+), 4 deletions(-)
 create mode 100644 tests/data/SPECS/suicidal.spec

diff --git a/lib/fsm.c b/lib/fsm.c
index 138b55297..8a4ecaf05 100644
--- a/lib/fsm.c
+++ b/lib/fsm.c
@@ -920,10 +920,6 @@ int rpmPackageFilesInstall(rpmts ts, rpmte te, rpmfiles files,
         if (!skip) {
 	    int setmeta = 1;
 
-	    /* When touching we don't need any of this... */
-	    if (action == FA_TOUCH)
-		goto touch;
-
 	    /* Directories replacing something need early backup */
 	    if (!suffix) {
 		rc = fsmBackup(fi, action);
@@ -935,6 +931,17 @@ int rpmPackageFilesInstall(rpmts ts, rpmte te, rpmfiles files,
 		rc = RPMERR_ENOENT;
 	    }
 
+	    /* See if the file was removed while our attention was elsewhere */
+	    if (rc == RPMERR_ENOENT && action == FA_TOUCH) {
+		rpmlog(RPMLOG_DEBUG, "file %s vanished unexpectedly\n", fpath);
+		action = FA_CREATE;
+		fsmDebug(fpath, action, &sb);
+	    }
+
+	    /* When touching we don't need any of this... */
+	    if (action == FA_TOUCH)
+		goto touch;
+
             if (S_ISREG(sb.st_mode)) {
 		if (rc == RPMERR_ENOENT) {
 		    rc = fsmMkfile(fi, fpath, files, psm, nodigest,
diff --git a/tests/Makefile.am b/tests/Makefile.am
index 7a6641d4f..d540e2a7b 100644
--- a/tests/Makefile.am
+++ b/tests/Makefile.am
@@ -59,6 +59,7 @@ EXTRA_DIST += data/SPECS/verifyscript.spec
 EXTRA_DIST += data/SPECS/fakeshell.spec
 EXTRA_DIST += data/SPECS/scripts.spec
 EXTRA_DIST += data/SPECS/selfconflict.spec
+EXTRA_DIST += data/SPECS/suicidal.spec
 EXTRA_DIST += data/SPECS/replacetest.spec
 EXTRA_DIST += data/SPECS/triggers.spec
 EXTRA_DIST += data/SPECS/filetriggers.spec
diff --git a/tests/data/SPECS/suicidal.spec b/tests/data/SPECS/suicidal.spec
new file mode 100644
index 000000000..77d17d8c9
--- /dev/null
+++ b/tests/data/SPECS/suicidal.spec
@@ -0,0 +1,19 @@
+Name: suicidal
+Version: 1
+Release: %{rel}
+License: GPL
+Group: Testing
+Summary: Testing suicidal package behavior
+BuildArch: noarch
+
+%description
+
+%build
+mkdir -p %{buildroot}/opt
+echo shoot > %{buildroot}/opt/foot
+
+%pre -p <lua>
+os.remove('/opt/foot')
+
+%files
+/opt/foot
diff --git a/tests/rpmi.at b/tests/rpmi.at
index e8d6e9b7a..71e17821b 100644
--- a/tests/rpmi.at
+++ b/tests/rpmi.at
@@ -711,3 +711,59 @@ runroot rpm -e testdoc
 [])
 AT_CLEANUP
 
+AT_SETUP([rpm -i --excludeartifacts])
+AT_KEYWORDS([install])
+RPMDB_INIT
+runroot rpmbuild --quiet -bb /data/SPECS/vattrtest.spec
+
+AT_CHECK([
+RPMDB_INIT
+runroot rpm -i --excludeartifacts /build/RPMS/noarch/vattrtest-1.0-1.noarch.rpm
+test -e ${RPMTEST}/opt/vattrtest/a && exit 1
+runroot rpm -e vattrtest
+runroot rpm -i /build/RPMS/noarch/vattrtest-1.0-1.noarch.rpm
+test -e ${RPMTEST}/opt/vattrtest/a || exit 1
+],
+[0],
+[],
+[])
+AT_CLEANUP
+
+AT_SETUP([rpm -U <suicidal>])
+AT_KEYWORDS([install])
+RPMDB_INIT
+
+for r in 1 2; do
+    runroot rpmbuild -bb --quiet \
+		--define "rel ${r}" \
+		/data/SPECS/suicidal.spec
+done
+
+AT_CHECK([
+RPMDB_INIT
+
+for r in 1 2; do
+    runroot rpm -U \
+	--define "_minimize_writes 0" \
+	/build/RPMS/noarch/suicidal-1-${r}.noarch.rpm
+done
+runroot rpm -V --nouser --nogroup suicidal
+],
+[0],
+[],
+[])
+
+AT_CHECK([
+RPMDB_INIT
+
+for r in 1 2; do
+    runroot rpm -U \
+	--define "_minimize_writes 1" \
+	/build/RPMS/noarch/suicidal-1-${r}.noarch.rpm
+done
+runroot rpm -V --nouser --nogroup suicidal
+],
+[0],
+[],
+[])
+AT_CLEANUP
-- 
2.34.1


From e78ea489eeed84cdee9f5cedcbbf35cfcf1a70b6 Mon Sep 17 00:00:00 2001
From: Panu Matilainen <pmatilai@redhat.com>
Date: Wed, 10 Feb 2021 09:47:19 +0200
Subject: [PATCH 05/10] Refactor file install and remove around a common struct

Collect the common state info into a struct shared by both file install
and remove, update code accordingly. The change looks much more drastic
than it is - it's just adding fp-> prefix to a lot of places.
While we're at it, remember the state data throughout the operation.

No functional changes here, just paving way for the next steps which
will look clearer with these pre-requisites in place.
---
 lib/fsm.c | 158 +++++++++++++++++++++++++++++-------------------------
 1 file changed, 85 insertions(+), 73 deletions(-)

diff --git a/lib/fsm.c b/lib/fsm.c
index 8a4ecaf05..ab15c2bf3 100644
--- a/lib/fsm.c
+++ b/lib/fsm.c
@@ -38,6 +38,14 @@ static int strict_erasures = 0;
 #define _dirPerms 0755
 #define _filePerms 0644
 
+struct filedata_s {
+    int skip;
+    rpmFileAction action;
+    const char *suffix;
+    char *fpath;
+    struct stat sb;
+};
+
 /* 
  * XXX Forward declarations for previously exported functions to avoid moving 
  * things around needlessly 
@@ -864,19 +872,16 @@ int rpmPackageFilesInstall(rpmts ts, rpmte te, rpmfiles files,
     rpmfi fi = rpmfiNewArchiveReader(payload, files, RPMFI_ITER_READ_ARCHIVE);
     rpmfs fs = rpmteGetFileStates(te);
     rpmPlugins plugins = rpmtsPlugins(ts);
-    struct stat sb;
     int saveerrno = errno;
     int rc = 0;
     int fx = -1;
+    int fc = rpmfilesFC(files);
     int nodigest = (rpmtsFlags(ts) & RPMTRANS_FLAG_NOFILEDIGEST) ? 1 : 0;
     int nofcaps = (rpmtsFlags(ts) & RPMTRANS_FLAG_NOCAPS) ? 1 : 0;
     int firsthardlink = -1;
     FD_t firstlinkfile = NULL;
-    int skip;
-    rpmFileAction action;
     char *tid = NULL;
-    const char *suffix;
-    char *fpath = NULL;
+    struct filedata_s *fdata = xcalloc(fc, sizeof(*fdata));
 
     if (fi == NULL) {
 	rc = RPMERR_BAD_MAGIC;
@@ -890,96 +895,99 @@ int rpmPackageFilesInstall(rpmts ts, rpmte te, rpmfiles files,
     rc = fsmMkdirs(files, fs, plugins);
 
     while (!rc && (fx = rpmfiNext(fi)) >= 0) {
-	action = rpmfsGetAction(fs, fx);
-	skip = XFA_SKIPPING(action);
-	if (action != FA_TOUCH) {
-	    suffix = S_ISDIR(rpmfiFMode(fi)) ? NULL : tid;
+	struct filedata_s *fp = &fdata[fx];
+	fp->action = rpmfsGetAction(fs, fx);
+	fp->skip = XFA_SKIPPING(fp->action);
+	if (fp->action != FA_TOUCH) {
+	    fp->suffix = S_ISDIR(rpmfiFMode(fi)) ? NULL : tid;
 	} else {
-	    suffix = NULL;
+	    fp->suffix = NULL;
 	}
-	fpath = fsmFsPath(fi, suffix);
+	fp->fpath = fsmFsPath(fi, fp->suffix);
 
 	/* Remap file perms, owner, and group. */
-	rc = rpmfiStat(fi, 1, &sb);
+	rc = rpmfiStat(fi, 1, &fp->sb);
 
-	fsmDebug(fpath, action, &sb);
+	fsmDebug(fp->fpath, fp->action, &fp->sb);
 
         /* Exit on error. */
         if (rc)
             break;
 
 	/* Run fsm file pre hook for all plugins */
-	rc = rpmpluginsCallFsmFilePre(plugins, fi, fpath,
-				      sb.st_mode, action);
+	rc = rpmpluginsCallFsmFilePre(plugins, fi, fp->fpath,
+				      fp->sb.st_mode, fp->action);
 	if (rc) {
-	    skip = 1;
+	    fp->skip = 1;
 	} else {
 	    setFileState(fs, fx);
 	}
 
-        if (!skip) {
+        if (!fp->skip) {
 	    int setmeta = 1;
 
 	    /* Directories replacing something need early backup */
-	    if (!suffix) {
-		rc = fsmBackup(fi, action);
+	    if (!fp->suffix) {
+		rc = fsmBackup(fi, fp->action);
 	    }
 	    /* Assume file does't exist when tmp suffix is in use */
-	    if (!suffix) {
-		rc = fsmVerify(fpath, fi);
+	    if (!fp->suffix) {
+		rc = fsmVerify(fp->fpath, fi);
 	    } else {
 		rc = RPMERR_ENOENT;
 	    }
 
 	    /* See if the file was removed while our attention was elsewhere */
-	    if (rc == RPMERR_ENOENT && action == FA_TOUCH) {
-		rpmlog(RPMLOG_DEBUG, "file %s vanished unexpectedly\n", fpath);
-		action = FA_CREATE;
-		fsmDebug(fpath, action, &sb);
+	    if (rc == RPMERR_ENOENT && fp->action == FA_TOUCH) {
+		rpmlog(RPMLOG_DEBUG, "file %s vanished unexpectedly\n",
+			fp->fpath);
+		fp->action = FA_CREATE;
+		fsmDebug(fp->fpath, fp->action, &fp->sb);
 	    }
 
 	    /* When touching we don't need any of this... */
-	    if (action == FA_TOUCH)
+	    if (fp->action == FA_TOUCH)
 		goto touch;
 
-            if (S_ISREG(sb.st_mode)) {
+            if (S_ISREG(fp->sb.st_mode)) {
 		if (rc == RPMERR_ENOENT) {
-		    rc = fsmMkfile(fi, fpath, files, psm, nodigest,
+		    rc = fsmMkfile(fi, fp->fpath, files, psm, nodigest,
 				   &setmeta, &firsthardlink, &firstlinkfile);
 		}
-            } else if (S_ISDIR(sb.st_mode)) {
+            } else if (S_ISDIR(fp->sb.st_mode)) {
                 if (rc == RPMERR_ENOENT) {
-                    mode_t mode = sb.st_mode;
+                    mode_t mode = fp->sb.st_mode;
                     mode &= ~07777;
                     mode |=  00700;
-                    rc = fsmMkdir(fpath, mode);
+                    rc = fsmMkdir(fp->fpath, mode);
                 }
-            } else if (S_ISLNK(sb.st_mode)) {
+            } else if (S_ISLNK(fp->sb.st_mode)) {
 		if (rc == RPMERR_ENOENT) {
-		    rc = fsmSymlink(rpmfiFLink(fi), fpath);
+		    rc = fsmSymlink(rpmfiFLink(fi), fp->fpath);
 		}
-            } else if (S_ISFIFO(sb.st_mode)) {
+            } else if (S_ISFIFO(fp->sb.st_mode)) {
                 /* This mimics cpio S_ISSOCK() behavior but probably isn't right */
                 if (rc == RPMERR_ENOENT) {
-                    rc = fsmMkfifo(fpath, 0000);
+                    rc = fsmMkfifo(fp->fpath, 0000);
                 }
-            } else if (S_ISCHR(sb.st_mode) ||
-                       S_ISBLK(sb.st_mode) ||
-                       S_ISSOCK(sb.st_mode))
+            } else if (S_ISCHR(fp->sb.st_mode) ||
+                       S_ISBLK(fp->sb.st_mode) ||
+                       S_ISSOCK(fp->sb.st_mode))
             {
                 if (rc == RPMERR_ENOENT) {
-                    rc = fsmMknod(fpath, sb.st_mode, sb.st_rdev);
+                    rc = fsmMknod(fp->fpath, fp->sb.st_mode, fp->sb.st_rdev);
                 }
             } else {
                 /* XXX Special case /dev/log, which shouldn't be packaged anyways */
-                if (!IS_DEV_LOG(fpath))
+                if (!IS_DEV_LOG(fp->fpath))
                     rc = RPMERR_UNKNOWN_FILETYPE;
             }
 
 touch:
 	    /* Set permissions, timestamps etc for non-hardlink entries */
 	    if (!rc && setmeta) {
-		rc = fsmSetmeta(fpath, fi, plugins, action, &sb, nofcaps);
+		rc = fsmSetmeta(fp->fpath, fi, plugins, fp->action,
+				&fp->sb, nofcaps);
 	    }
         } else if (firsthardlink >= 0 && rpmfiArchiveHasContent(fi)) {
 	    /* On FA_TOUCH no hardlinks are created thus this is skipped. */
@@ -991,10 +999,10 @@ touch:
 	}
 
         if (rc) {
-            if (!skip) {
+            if (!fp->skip) {
                 /* XXX only erase if temp fn w suffix is in use */
-                if (suffix) {
-		    (void) fsmRemove(fpath, sb.st_mode);
+                if (fp->suffix) {
+		    (void) fsmRemove(fp->fpath, fp->sb.st_mode);
                 }
                 errno = saveerrno;
             }
@@ -1002,23 +1010,22 @@ touch:
 	    /* Notify on success. */
 	    rpmpsmNotify(psm, RPMCALLBACK_INST_PROGRESS, rpmfiArchiveTell(fi));
 
-	    if (!skip) {
+	    if (!fp->skip) {
 		/* Backup file if needed. Directories are handled earlier */
-		if (suffix)
-		    rc = fsmBackup(fi, action);
+		if (fp->suffix)
+		    rc = fsmBackup(fi, fp->action);
 
 		if (!rc)
-		    rc = fsmCommit(&fpath, fi, action, suffix);
+		    rc = fsmCommit(&fp->fpath, fi, fp->action, fp->suffix);
 	    }
 	}
 
 	if (rc)
-	    *failedFile = xstrdup(fpath);
+	    *failedFile = xstrdup(fp->fpath);
 
 	/* Run fsm file post hook for all plugins */
-	rpmpluginsCallFsmFilePost(plugins, fi, fpath,
-				  sb.st_mode, action, rc);
-	fpath = _free(fpath);
+	rpmpluginsCallFsmFilePost(plugins, fi, fp->fpath,
+				  fp->sb.st_mode, fp->action, rc);
     }
 
     if (!rc && fx != RPMERR_ITER_END)
@@ -1034,7 +1041,9 @@ exit:
     rpmfiFree(fi);
     Fclose(payload);
     free(tid);
-    free(fpath);
+    for (int i = 0; i < fc; i++)
+	free(fdata[i].fpath);
+    free(fdata);
 
     return rc;
 }
@@ -1046,29 +1055,31 @@ int rpmPackageFilesRemove(rpmts ts, rpmte te, rpmfiles files,
     rpmfi fi = rpmfilesIter(files, RPMFI_ITER_BACK);
     rpmfs fs = rpmteGetFileStates(te);
     rpmPlugins plugins = rpmtsPlugins(ts);
-    struct stat sb;
+    int fc = rpmfilesFC(files);
+    int fx = -1;
+    struct filedata_s *fdata = xcalloc(fc, sizeof(*fdata));
     int rc = 0;
-    char *fpath = NULL;
 
-    while (!rc && rpmfiNext(fi) >= 0) {
-	rpmFileAction action = rpmfsGetAction(fs, rpmfiFX(fi));
-	fpath = fsmFsPath(fi, NULL);
-	rc = fsmStat(fpath, 1, &sb);
+    while (!rc && (fx = rpmfiNext(fi)) >= 0) {
+	struct filedata_s *fp = &fdata[fx];
+	fp->action = rpmfsGetAction(fs, rpmfiFX(fi));
+	fp->fpath = fsmFsPath(fi, NULL);
+	rc = fsmStat(fp->fpath, 1, &fp->sb);
 
-	fsmDebug(fpath, action, &sb);
+	fsmDebug(fp->fpath, fp->action, &fp->sb);
 
 	/* Run fsm file pre hook for all plugins */
-	rc = rpmpluginsCallFsmFilePre(plugins, fi, fpath,
-				      sb.st_mode, action);
+	rc = rpmpluginsCallFsmFilePre(plugins, fi, fp->fpath,
+				      fp->sb.st_mode, fp->action);
 
-	if (!XFA_SKIPPING(action))
-	    rc = fsmBackup(fi, action);
+	if (!XFA_SKIPPING(fp->action))
+	    rc = fsmBackup(fi, fp->action);
 
         /* Remove erased files. */
-        if (action == FA_ERASE) {
+        if (fp->action == FA_ERASE) {
 	    int missingok = (rpmfiFFlags(fi) & (RPMFILE_MISSINGOK | RPMFILE_GHOST));
 
-	    rc = fsmRemove(fpath, sb.st_mode);
+	    rc = fsmRemove(fp->fpath, fp->sb.st_mode);
 
 	    /*
 	     * Missing %ghost or %missingok entries are not errors.
@@ -1093,20 +1104,20 @@ int rpmPackageFilesRemove(rpmts ts, rpmte te, rpmfiles files,
 	    if (rc) {
 		int lvl = strict_erasures ? RPMLOG_ERR : RPMLOG_WARNING;
 		rpmlog(lvl, _("%s %s: remove failed: %s\n"),
-			S_ISDIR(sb.st_mode) ? _("directory") : _("file"),
-			fpath, strerror(errno));
+			S_ISDIR(fp->sb.st_mode) ? _("directory") : _("file"),
+			fp->fpath, strerror(errno));
             }
         }
 
 	/* Run fsm file post hook for all plugins */
-	rpmpluginsCallFsmFilePost(plugins, fi, fpath,
-				  sb.st_mode, action, rc);
+	rpmpluginsCallFsmFilePost(plugins, fi, fp->fpath,
+				  fp->sb.st_mode, fp->action, rc);
 
         /* XXX Failure to remove is not (yet) cause for failure. */
         if (!strict_erasures) rc = 0;
 
 	if (rc)
-	    *failedFile = xstrdup(fpath);
+	    *failedFile = xstrdup(fp->fpath);
 
 	if (rc == 0) {
 	    /* Notify on success. */
@@ -1114,10 +1125,11 @@ int rpmPackageFilesRemove(rpmts ts, rpmte te, rpmfiles files,
 	    rpm_loff_t amount = rpmfiFC(fi) - rpmfiFX(fi);
 	    rpmpsmNotify(psm, RPMCALLBACK_UNINST_PROGRESS, amount);
 	}
-	fpath = _free(fpath);
     }
 
-    free(fpath);
+    for (int i = 0; i < fc; i++)
+	free(fdata[i].fpath);
+    free(fdata);
     rpmfiFree(fi);
 
     return rc;
-- 
2.34.1


From 215525131b0e4e015d83c25828700d7531fec6dd Mon Sep 17 00:00:00 2001
From: Panu Matilainen <pmatilai@redhat.com>
Date: Wed, 10 Feb 2021 10:08:27 +0200
Subject: [PATCH 06/10] Refactor fsmMkfile() to take advantage of the new state
 struct

Move setmeta into the struct too (we'll want this later anyhow),
and now we only need to pass the struct to fsmMkfile(). One less
argument to pass around, it has way too many still.

No functional changes.
---
 lib/fsm.c | 22 +++++++++++-----------
 1 file changed, 11 insertions(+), 11 deletions(-)

diff --git a/lib/fsm.c b/lib/fsm.c
index ab15c2bf3..6c873206c 100644
--- a/lib/fsm.c
+++ b/lib/fsm.c
@@ -39,6 +39,7 @@ static int strict_erasures = 0;
 #define _filePerms 0644
 
 struct filedata_s {
+    int setmeta;
     int skip;
     rpmFileAction action;
     const char *suffix;
@@ -279,8 +280,8 @@ exit:
     return rc;
 }
 
-static int fsmMkfile(rpmfi fi, const char *dest, rpmfiles files,
-		     rpmpsm psm, int nodigest, int *setmeta,
+static int fsmMkfile(rpmfi fi, struct filedata_s *fp, rpmfiles files,
+		     rpmpsm psm, int nodigest,
 		     int * firsthardlink, FD_t *firstlinkfile)
 {
     int rc = 0;
@@ -290,11 +291,11 @@ static int fsmMkfile(rpmfi fi, const char *dest, rpmfiles files,
 	/* Create first hardlinked file empty */
 	if (*firsthardlink < 0) {
 	    *firsthardlink = rpmfiFX(fi);
-	    rc = wfd_open(firstlinkfile, dest);
+	    rc = wfd_open(firstlinkfile, fp->fpath);
 	} else {
 	    /* Create hard links for others */
 	    char *fn = rpmfilesFN(files, *firsthardlink);
-	    rc = link(fn, dest);
+	    rc = link(fn, fp->fpath);
 	    if (rc < 0) {
 		rc = RPMERR_LINK_FAILED;
 	    }
@@ -305,14 +306,14 @@ static int fsmMkfile(rpmfi fi, const char *dest, rpmfiles files,
        existing) file with content */
     if (numHardlinks<=1) {
 	if (!rc)
-	    rc = expandRegular(fi, dest, psm, nodigest);
+	    rc = expandRegular(fi, fp->fpath, psm, nodigest);
     } else if (rpmfiArchiveHasContent(fi)) {
 	if (!rc)
 	    rc = rpmfiArchiveReadToFilePsm(fi, *firstlinkfile, nodigest, psm);
 	wfd_close(firstlinkfile);
 	*firsthardlink = -1;
     } else {
-	*setmeta = 0;
+	fp->setmeta = 0;
     }
 
     return rc;
@@ -898,6 +899,7 @@ int rpmPackageFilesInstall(rpmts ts, rpmte te, rpmfiles files,
 	struct filedata_s *fp = &fdata[fx];
 	fp->action = rpmfsGetAction(fs, fx);
 	fp->skip = XFA_SKIPPING(fp->action);
+	fp->setmeta = 1;
 	if (fp->action != FA_TOUCH) {
 	    fp->suffix = S_ISDIR(rpmfiFMode(fi)) ? NULL : tid;
 	} else {
@@ -924,8 +926,6 @@ int rpmPackageFilesInstall(rpmts ts, rpmte te, rpmfiles files,
 	}
 
         if (!fp->skip) {
-	    int setmeta = 1;
-
 	    /* Directories replacing something need early backup */
 	    if (!fp->suffix) {
 		rc = fsmBackup(fi, fp->action);
@@ -951,8 +951,8 @@ int rpmPackageFilesInstall(rpmts ts, rpmte te, rpmfiles files,
 
             if (S_ISREG(fp->sb.st_mode)) {
 		if (rc == RPMERR_ENOENT) {
-		    rc = fsmMkfile(fi, fp->fpath, files, psm, nodigest,
-				   &setmeta, &firsthardlink, &firstlinkfile);
+		    rc = fsmMkfile(fi, fp, files, psm, nodigest,
+				   &firsthardlink, &firstlinkfile);
 		}
             } else if (S_ISDIR(fp->sb.st_mode)) {
                 if (rc == RPMERR_ENOENT) {
@@ -985,7 +985,7 @@ int rpmPackageFilesInstall(rpmts ts, rpmte te, rpmfiles files,
 
 touch:
 	    /* Set permissions, timestamps etc for non-hardlink entries */
-	    if (!rc && setmeta) {
+	    if (!rc && fp->setmeta) {
 		rc = fsmSetmeta(fp->fpath, fi, plugins, fp->action,
 				&fp->sb, nofcaps);
 	    }
-- 
2.34.1


From 062167c2532a066301b15a0e85a1ac6cb6c07472 Mon Sep 17 00:00:00 2001
From: Panu Matilainen <pmatilai@redhat.com>
Date: Wed, 10 Feb 2021 10:24:22 +0200
Subject: [PATCH 07/10] Clarify file installation temporary suffix rule

We only use a temporary suffix for regular files that we are actually
creating, skipped and touched files should not have it. Add XFA_CREATING()
macro to accomppany XFA_SKIPPING() to easily check whether the file
is being created or something else.

No functional changes but makes the logic clearer.
---
 lib/fsm.c      | 5 +----
 lib/rpmfiles.h | 3 +++
 2 files changed, 4 insertions(+), 4 deletions(-)

diff --git a/lib/fsm.c b/lib/fsm.c
index 6c873206c..e4842a200 100644
--- a/lib/fsm.c
+++ b/lib/fsm.c
@@ -900,11 +900,8 @@ int rpmPackageFilesInstall(rpmts ts, rpmte te, rpmfiles files,
 	fp->action = rpmfsGetAction(fs, fx);
 	fp->skip = XFA_SKIPPING(fp->action);
 	fp->setmeta = 1;
-	if (fp->action != FA_TOUCH) {
+	if (XFA_CREATING(fp->action))
 	    fp->suffix = S_ISDIR(rpmfiFMode(fi)) ? NULL : tid;
-	} else {
-	    fp->suffix = NULL;
-	}
 	fp->fpath = fsmFsPath(fi, fp->suffix);
 
 	/* Remap file perms, owner, and group. */
diff --git a/lib/rpmfiles.h b/lib/rpmfiles.h
index 64b33281a..7f3f82662 100644
--- a/lib/rpmfiles.h
+++ b/lib/rpmfiles.h
@@ -90,6 +90,9 @@ typedef enum rpmFileAction_e {
 #define XFA_SKIPPING(_a)	\
     ((_a) == FA_SKIP || (_a) == FA_SKIPNSTATE || (_a) == FA_SKIPNETSHARED || (_a) == FA_SKIPCOLOR)
 
+#define XFA_CREATING(_a)	\
+    ((_a) == FA_CREATE || (_a) == FA_BACKUP || (_a) == FA_SAVE || (_a) == FA_ALTNAME)
+
 /**
  * We pass these around as an array with a sentinel.
  */
-- 
2.34.1


From b3c6a3358dc4ec72fda0a229c22a5b79ac392848 Mon Sep 17 00:00:00 2001
From: Panu Matilainen <pmatilai@redhat.com>
Date: Wed, 10 Feb 2021 10:24:22 +0200
Subject: [PATCH 08/10] Clarify file installation temporary suffix rule

We only use a temporary suffix for regular files that we are actually
creating, skipped and touched files should not have it. Add XFA_CREATING()
macro to accomppany XFA_SKIPPING() to easily check whether the file
is being created or something else.

No functional changes but makes the logic clearer.
---
 lib/fsm.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/lib/fsm.c b/lib/fsm.c
index e4842a200..4d7c03895 100644
--- a/lib/fsm.c
+++ b/lib/fsm.c
@@ -900,8 +900,8 @@ int rpmPackageFilesInstall(rpmts ts, rpmte te, rpmfiles files,
 	fp->action = rpmfsGetAction(fs, fx);
 	fp->skip = XFA_SKIPPING(fp->action);
 	fp->setmeta = 1;
-	if (XFA_CREATING(fp->action))
-	    fp->suffix = S_ISDIR(rpmfiFMode(fi)) ? NULL : tid;
+	if (XFA_CREATING(fp->action) && !S_ISDIR(rpmfiFMode(fi)))
+	    fp->suffix = tid;
 	fp->fpath = fsmFsPath(fi, fp->suffix);
 
 	/* Remap file perms, owner, and group. */
-- 
2.34.1


From 9097021b19c658ff55f7515878bae0592fd86377 Mon Sep 17 00:00:00 2001
From: Panu Matilainen <pmatilai@redhat.com>
Date: Wed, 10 Feb 2021 11:25:10 +0200
Subject: [PATCH 09/10] Handle hardlink tracking with a file state pointer

No functional changes, just makes it a little cleaner as firstlink now
points to the actual file data instead of a index number somewhere.
---
 lib/fsm.c | 20 +++++++++-----------
 1 file changed, 9 insertions(+), 11 deletions(-)

diff --git a/lib/fsm.c b/lib/fsm.c
index 4d7c03895..cef17e669 100644
--- a/lib/fsm.c
+++ b/lib/fsm.c
@@ -282,24 +282,22 @@ exit:
 
 static int fsmMkfile(rpmfi fi, struct filedata_s *fp, rpmfiles files,
 		     rpmpsm psm, int nodigest,
-		     int * firsthardlink, FD_t *firstlinkfile)
+		     struct filedata_s ** firstlink, FD_t *firstlinkfile)
 {
     int rc = 0;
     int numHardlinks = rpmfiFNlink(fi);
 
     if (numHardlinks > 1) {
 	/* Create first hardlinked file empty */
-	if (*firsthardlink < 0) {
-	    *firsthardlink = rpmfiFX(fi);
+	if (*firstlink == NULL) {
+	    *firstlink = fp;
 	    rc = wfd_open(firstlinkfile, fp->fpath);
 	} else {
 	    /* Create hard links for others */
-	    char *fn = rpmfilesFN(files, *firsthardlink);
-	    rc = link(fn, fp->fpath);
+	    rc = link((*firstlink)->fpath, fp->fpath);
 	    if (rc < 0) {
 		rc = RPMERR_LINK_FAILED;
 	    }
-	    free(fn);
 	}
     }
     /* Write normal files or fill the last hardlinked (already
@@ -311,7 +309,7 @@ static int fsmMkfile(rpmfi fi, struct filedata_s *fp, rpmfiles files,
 	if (!rc)
 	    rc = rpmfiArchiveReadToFilePsm(fi, *firstlinkfile, nodigest, psm);
 	wfd_close(firstlinkfile);
-	*firsthardlink = -1;
+	*firstlink = NULL;
     } else {
 	fp->setmeta = 0;
     }
@@ -879,10 +877,10 @@ int rpmPackageFilesInstall(rpmts ts, rpmte te, rpmfiles files,
     int fc = rpmfilesFC(files);
     int nodigest = (rpmtsFlags(ts) & RPMTRANS_FLAG_NOFILEDIGEST) ? 1 : 0;
     int nofcaps = (rpmtsFlags(ts) & RPMTRANS_FLAG_NOCAPS) ? 1 : 0;
-    int firsthardlink = -1;
     FD_t firstlinkfile = NULL;
     char *tid = NULL;
     struct filedata_s *fdata = xcalloc(fc, sizeof(*fdata));
+    struct filedata_s *firstlink = NULL;
 
     if (fi == NULL) {
 	rc = RPMERR_BAD_MAGIC;
@@ -949,7 +947,7 @@ int rpmPackageFilesInstall(rpmts ts, rpmte te, rpmfiles files,
             if (S_ISREG(fp->sb.st_mode)) {
 		if (rc == RPMERR_ENOENT) {
 		    rc = fsmMkfile(fi, fp, files, psm, nodigest,
-				   &firsthardlink, &firstlinkfile);
+				   &firstlink, &firstlinkfile);
 		}
             } else if (S_ISDIR(fp->sb.st_mode)) {
                 if (rc == RPMERR_ENOENT) {
@@ -986,13 +984,13 @@ touch:
 		rc = fsmSetmeta(fp->fpath, fi, plugins, fp->action,
 				&fp->sb, nofcaps);
 	    }
-        } else if (firsthardlink >= 0 && rpmfiArchiveHasContent(fi)) {
+        } else if (firstlink && rpmfiArchiveHasContent(fi)) {
 	    /* On FA_TOUCH no hardlinks are created thus this is skipped. */
 	    /* we skip the hard linked file containing the content */
 	    /* write the content to the first used instead */
 	    rc = rpmfiArchiveReadToFilePsm(fi, firstlinkfile, nodigest, psm);
 	    wfd_close(&firstlinkfile);
-	    firsthardlink = -1;
+	    firstlink = NULL;
 	}
 
         if (rc) {
-- 
2.34.1


From 357afe98316cb643a0757163ddbc276a071f1a18 Mon Sep 17 00:00:00 2001
From: Panu Matilainen <pmatilai@redhat.com>
Date: Wed, 10 Feb 2021 14:15:33 +0200
Subject: [PATCH 10/10] Handle file install failures more gracefully

Run the file installation in multiple stages:
1) gather intel
2) unpack the archive to temporary files
3) set file metadatas
4) commit files to final location
5) mop up leftovers on failure

This means we no longer leave behind a trail of untracked, potentially
harmful junk on installation failure.

If commit step fails the package can still be left in an inconsistent stage,
this could be further improved by first backing up old files to temporary
location to allow undo on failure, but leaving that for some other day.
Also unowned directories will still be left behind.

And yes, this is a somewhat scary change as it's the biggest ever change
to how rpm lays down files on install. Adopt the hardlink test spec
over to install tests and add some more tests for the new behavior.

Fixes: #967 (+ multiple reports over the years)
---
 lib/fsm.c                       | 147 ++++++++++++++++++++------------
 tests/data/SPECS/hlinktest.spec |   4 +
 tests/rpmbuild.at               |  33 -------
 tests/rpmi.at                   |  92 ++++++++++++++++++++
 4 files changed, 189 insertions(+), 87 deletions(-)

diff --git a/lib/fsm.c b/lib/fsm.c
index cef17e669..82af6f39f 100644
--- a/lib/fsm.c
+++ b/lib/fsm.c
@@ -38,7 +38,17 @@ static int strict_erasures = 0;
 #define _dirPerms 0755
 #define _filePerms 0644
 
+enum filestage_e {
+    FILE_COMMIT = -1,
+    FILE_NONE   = 0,
+    FILE_PRE    = 1,
+    FILE_UNPACK = 2,
+    FILE_PREP   = 3,
+    FILE_POST   = 4,
+};
+
 struct filedata_s {
+    int stage;
     int setmeta;
     int skip;
     rpmFileAction action;
@@ -868,10 +878,9 @@ int rpmPackageFilesInstall(rpmts ts, rpmte te, rpmfiles files,
               rpmpsm psm, char ** failedFile)
 {
     FD_t payload = rpmtePayload(te);
-    rpmfi fi = rpmfiNewArchiveReader(payload, files, RPMFI_ITER_READ_ARCHIVE);
+    rpmfi fi = NULL;
     rpmfs fs = rpmteGetFileStates(te);
     rpmPlugins plugins = rpmtsPlugins(ts);
-    int saveerrno = errno;
     int rc = 0;
     int fx = -1;
     int fc = rpmfilesFC(files);
@@ -882,20 +891,17 @@ int rpmPackageFilesInstall(rpmts ts, rpmte te, rpmfiles files,
     struct filedata_s *fdata = xcalloc(fc, sizeof(*fdata));
     struct filedata_s *firstlink = NULL;
 
-    if (fi == NULL) {
-	rc = RPMERR_BAD_MAGIC;
-	goto exit;
-    }
-
     /* transaction id used for temporary path suffix while installing */
     rasprintf(&tid, ";%08x", (unsigned)rpmtsGetTid(ts));
 
-    /* Detect and create directories not explicitly in package. */
-    rc = fsmMkdirs(files, fs, plugins);
-
+    /* Collect state data for the whole operation */
+    fi = rpmfilesIter(files, RPMFI_ITER_FWD);
     while (!rc && (fx = rpmfiNext(fi)) >= 0) {
 	struct filedata_s *fp = &fdata[fx];
-	fp->action = rpmfsGetAction(fs, fx);
+	if (rpmfiFFlags(fi) & RPMFILE_GHOST)
+            fp->action = FA_SKIP;
+	else
+	    fp->action = rpmfsGetAction(fs, fx);
 	fp->skip = XFA_SKIPPING(fp->action);
 	fp->setmeta = 1;
 	if (XFA_CREATING(fp->action) && !S_ISDIR(rpmfiFMode(fi)))
@@ -905,20 +911,32 @@ int rpmPackageFilesInstall(rpmts ts, rpmte te, rpmfiles files,
 	/* Remap file perms, owner, and group. */
 	rc = rpmfiStat(fi, 1, &fp->sb);
 
+	setFileState(fs, fx);
 	fsmDebug(fp->fpath, fp->action, &fp->sb);
 
-        /* Exit on error. */
-        if (rc)
-            break;
-
 	/* Run fsm file pre hook for all plugins */
 	rc = rpmpluginsCallFsmFilePre(plugins, fi, fp->fpath,
 				      fp->sb.st_mode, fp->action);
-	if (rc) {
-	    fp->skip = 1;
-	} else {
-	    setFileState(fs, fx);
-	}
+	fp->stage = FILE_PRE;
+    }
+    fi = rpmfiFree(fi);
+
+    if (rc)
+	goto exit;
+
+    fi = rpmfiNewArchiveReader(payload, files, RPMFI_ITER_READ_ARCHIVE);
+    if (fi == NULL) {
+        rc = RPMERR_BAD_MAGIC;
+        goto exit;
+    }
+
+    /* Detect and create directories not explicitly in package. */
+    if (!rc)
+	rc = fsmMkdirs(files, fs, plugins);
+
+    /* Process the payload */
+    while (!rc && (fx = rpmfiNext(fi)) >= 0) {
+	struct filedata_s *fp = &fdata[fx];
 
         if (!fp->skip) {
 	    /* Directories replacing something need early backup */
@@ -942,7 +960,7 @@ int rpmPackageFilesInstall(rpmts ts, rpmte te, rpmfiles files,
 
 	    /* When touching we don't need any of this... */
 	    if (fp->action == FA_TOUCH)
-		goto touch;
+		continue;
 
             if (S_ISREG(fp->sb.st_mode)) {
 		if (rc == RPMERR_ENOENT) {
@@ -978,12 +996,6 @@ int rpmPackageFilesInstall(rpmts ts, rpmte te, rpmfiles files,
                     rc = RPMERR_UNKNOWN_FILETYPE;
             }
 
-touch:
-	    /* Set permissions, timestamps etc for non-hardlink entries */
-	    if (!rc && fp->setmeta) {
-		rc = fsmSetmeta(fp->fpath, fi, plugins, fp->action,
-				&fp->sb, nofcaps);
-	    }
         } else if (firstlink && rpmfiArchiveHasContent(fi)) {
 	    /* On FA_TOUCH no hardlinks are created thus this is skipped. */
 	    /* we skip the hard linked file containing the content */
@@ -993,47 +1005,74 @@ touch:
 	    firstlink = NULL;
 	}
 
-        if (rc) {
-            if (!fp->skip) {
-                /* XXX only erase if temp fn w suffix is in use */
-                if (fp->suffix) {
-		    (void) fsmRemove(fp->fpath, fp->sb.st_mode);
-                }
-                errno = saveerrno;
-            }
-        } else {
-	    /* Notify on success. */
+	/* Notify on success. */
+	if (rc)
+	    *failedFile = xstrdup(fp->fpath);
+	else
 	    rpmpsmNotify(psm, RPMCALLBACK_INST_PROGRESS, rpmfiArchiveTell(fi));
+	fp->stage = FILE_UNPACK;
+    }
+    fi = rpmfiFree(fi);
 
-	    if (!fp->skip) {
-		/* Backup file if needed. Directories are handled earlier */
-		if (fp->suffix)
-		    rc = fsmBackup(fi, fp->action);
+    if (!rc && fx < 0 && fx != RPMERR_ITER_END)
+	rc = fx;
 
-		if (!rc)
-		    rc = fsmCommit(&fp->fpath, fi, fp->action, fp->suffix);
-	    }
+    /* Set permissions, timestamps etc for non-hardlink entries */
+    fi = rpmfilesIter(files, RPMFI_ITER_FWD);
+    while (!rc && (fx = rpmfiNext(fi)) >= 0) {
+	struct filedata_s *fp = &fdata[fx];
+	if (!fp->skip && fp->setmeta) {
+	    rc = fsmSetmeta(fp->fpath, fi, plugins, fp->action,
+			    &fp->sb, nofcaps);
 	}
-
 	if (rc)
 	    *failedFile = xstrdup(fp->fpath);
+	fp->stage = FILE_PREP;
+    }
+    fi = rpmfiFree(fi);
 
-	/* Run fsm file post hook for all plugins */
-	rpmpluginsCallFsmFilePost(plugins, fi, fp->fpath,
-				  fp->sb.st_mode, fp->action, rc);
+    /* If all went well, commit files to final destination */
+    fi = rpmfilesIter(files, RPMFI_ITER_FWD);
+    while (!rc && (fx = rpmfiNext(fi)) >= 0) {
+	struct filedata_s *fp = &fdata[fx];
+
+	if (!fp->skip) {
+	    /* Backup file if needed. Directories are handled earlier */
+	    if (!rc && fp->suffix)
+		rc = fsmBackup(fi, fp->action);
+
+	    if (!rc)
+		rc = fsmCommit(&fp->fpath, fi, fp->action, fp->suffix);
+
+	    if (!rc)
+		fp->stage = FILE_COMMIT;
+	    else
+		*failedFile = xstrdup(fp->fpath);
+	}
     }
+    fi = rpmfiFree(fi);
 
-    if (!rc && fx != RPMERR_ITER_END)
-	rc = fx;
+    /* Walk backwards in case we need to erase */
+    fi = rpmfilesIter(files, RPMFI_ITER_BACK);
+    while ((fx = rpmfiNext(fi)) >= 0) {
+	struct filedata_s *fp = &fdata[fx];
+	/* Run fsm file post hook for all plugins for all processed files */
+	if (fp->stage) {
+	    rpmpluginsCallFsmFilePost(plugins, fi, fp->fpath,
+				      fp->sb.st_mode, fp->action, rc);
+	}
+
+	/* On failure, erase non-committed files */
+	if (rc && fp->stage > FILE_NONE && !fp->skip) {
+	    (void) fsmRemove(fp->fpath, fp->sb.st_mode);
+	}
+    }
 
     rpmswAdd(rpmtsOp(ts, RPMTS_OP_UNCOMPRESS), fdOp(payload, FDSTAT_READ));
     rpmswAdd(rpmtsOp(ts, RPMTS_OP_DIGEST), fdOp(payload, FDSTAT_DIGEST));
 
 exit:
-
-    /* No need to bother with close errors on read */
-    rpmfiArchiveClose(fi);
-    rpmfiFree(fi);
+    fi = rpmfiFree(fi);
     Fclose(payload);
     free(tid);
     for (int i = 0; i < fc; i++)
diff --git a/tests/data/SPECS/hlinktest.spec b/tests/data/SPECS/hlinktest.spec
index b3b8866fc..1f453524e 100644
--- a/tests/data/SPECS/hlinktest.spec
+++ b/tests/data/SPECS/hlinktest.spec
@@ -1,5 +1,6 @@
 %bcond_with unpackaged_dirs
 %bcond_with unpackaged_files
+%bcond_with owned_dir
 
 Summary:          Testing hard link behavior
 Name:             hlinktest
@@ -38,4 +39,7 @@ touch $RPM_BUILD_ROOT/toot
 
 %files
 %defattr(-,root,root)
+%if %{with owned_dir}
+%dir /foo
+%endif
 /foo/*
diff --git a/tests/rpmbuild.at b/tests/rpmbuild.at
index 4294fd97c..c99fa7a63 100644
--- a/tests/rpmbuild.at
+++ b/tests/rpmbuild.at
@@ -139,39 +139,6 @@ drwxrwxrwx zoot     zoot     /j/dir
 [])
 AT_CLEANUP
 
-# ------------------------------
-# hardlink tests
-AT_SETUP([rpmbuild hardlink])
-AT_KEYWORDS([build])
-AT_CHECK([
-RPMDB_CLEAR
-RPMDB_INIT
-rm -rf ${TOPDIR}
-
-runroot rpmbuild \
-  -bb --quiet /data/SPECS/hlinktest.spec
-
-runroot rpm -i /build/RPMS/noarch/hlinktest-1.0-1.noarch.rpm
-
-runroot rpm -q --qf "[[%{filenlinks} %{filenames}\n]]%{longsize}\n" hlinktest
-runroot rpm -V --nouser --nogroup hlinktest
-ls -i "${RPMTEST}"/foo/hello* | awk {'print $1'} | sort -u | wc -l
-
-],
-[0],
-[2 /foo/aaaa
-1 /foo/copyllo
-4 /foo/hello
-4 /foo/hello-bar
-4 /foo/hello-foo
-4 /foo/hello-world
-2 /foo/zzzz
-87
-1
-],
-[])
-AT_CLEANUP
-
 AT_SETUP([rpmbuild unpackaged files])
 AT_KEYWORDS([build])
 AT_CHECK([
diff --git a/tests/rpmi.at b/tests/rpmi.at
index 71e17821b..bf1606b6d 100644
--- a/tests/rpmi.at
+++ b/tests/rpmi.at
@@ -767,3 +767,95 @@ runroot rpm -V --nouser --nogroup suicidal
 [],
 [])
 AT_CLEANUP
+
+# ------------------------------
+# hardlink tests
+AT_SETUP([rpm -i hardlinks])
+AT_KEYWORDS([build install])
+RPMDB_INIT
+
+# Need a reproducable test package
+runroot rpmbuild \
+  --define "%optflags -O2 -g" \
+  --define "%_target_platform noarch-linux" \
+  --define "%_binary_payload w.ufdio" \
+  --define "%_buildhost localhost" \
+  --define "%use_source_date_epoch_as_buildtime 1" \
+  --define "%source_date_epoch_from_changelog 1" \
+  --define "%clamp_mtime_to_source_date_epoch 1" \
+  --with owned_dir \
+  -bb --quiet /data/SPECS/hlinktest.spec
+
+pkg="/build/RPMS/noarch/hlinktest-1.0-1.noarch.rpm"
+
+cp "${RPMTEST}/${pkg}" "${RPMTEST}/tmp/1.rpm"
+dd if=/dev/zero of="${RPMTEST}/tmp/1.rpm" \
+   conv=notrunc bs=1 seek=8180 count=6 2> /dev/null
+
+cp "${RPMTEST}/${pkg}" "${RPMTEST}/tmp/2.rpm"
+dd if=/dev/zero of="${RPMTEST}/tmp/2.rpm" \
+   conv=notrunc bs=1 seek=8150 count=6 2> /dev/null
+
+cp "${RPMTEST}/${pkg}" "${RPMTEST}/tmp/3.rpm"
+dd if=/dev/zero of="${RPMTEST}/tmp/3.rpm" \
+   conv=notrunc bs=1 seek=8050 count=6 2> /dev/null
+
+AT_CHECK([
+RPMDB_INIT
+runroot rpm -i --noverify /tmp/1.rpm
+# test that nothing of the contents remains after failure
+test -d "${RPMTEST}/foo"
+],
+[1],
+[],
+[error: unpacking of archive failed: cpio: Archive file not in header
+error: hlinktest-1.0-1.noarch: install failed
+])
+
+AT_CHECK([
+RPMDB_INIT
+runroot rpm -i --noverify /tmp/2.rpm
+# test that nothing of the contents remains after failure
+test -d "${RPMTEST}/foo"
+],
+[1],
+[],
+[error: unpacking of archive failed: cpio: Bad/unreadable  header
+error: hlinktest-1.0-1.noarch: install failed
+])
+
+AT_CHECK([
+RPMDB_INIT
+runroot rpm -i --noverify /tmp/3.rpm 2>&1| sed 's/;.*:/:/g'
+# test that nothing of the contents remains after failure
+test -d "${RPMTEST}/foo"
+],
+[1],
+[error: unpacking of archive failed on file /foo/hello-world: Digest mismatch
+error: hlinktest-1.0-1.noarch: install failed
+],
+[])
+
+AT_CHECK([
+RPMDB_INIT
+runroot rpm -i /build/RPMS/noarch/hlinktest-1.0-1.noarch.rpm
+runroot rpm -q --qf "[[%{filenlinks} %{filenames}\n]]%{longsize}\n" hlinktest
+ls -i "${RPMTEST}"/foo/hello* | awk {'print $1'} | sort -u | wc -l
+runroot rpm -e hlinktest
+
+],
+[0],
+[1 /foo
+2 /foo/aaaa
+1 /foo/copyllo
+4 /foo/hello
+4 /foo/hello-bar
+4 /foo/hello-foo
+4 /foo/hello-world
+2 /foo/zzzz
+87
+1
+],
+[])
+AT_CLEANUP
+
-- 
2.34.1