From 22d18785b2c41f1a8937b712920b4342f7596a7e Mon Sep 17 00:00:00 2001
From: Panu Matilainen <pmatilai@redhat.com>
Date: Mon, 8 Feb 2021 10:45:59 +0200
Subject: [PATCH 1/9] 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 35dcda081c..7c291adb02 100644
--- a/lib/fsm.c
+++ b/lib/fsm.c
@@ -841,6 +841,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;
@@ -862,17 +863,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;
@@ -896,7 +888,7 @@ int rpmPackageFilesInstall(rpmts ts, rpmte te, rpmfiles files,
if (rc) {
skip = 1;
} else {
- setFileState(fs, rpmfiFX(fi));
+ setFileState(fs, fx);
}
if (!skip) {
@@ -1005,6 +997,9 @@ int rpmPackageFilesInstall(rpmts ts, rpmte te, rpmfiles files,
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));
From c0dc57b820791dd76ce8baafac59b9a58ab0f0d3 Mon Sep 17 00:00:00 2001
From: Panu Matilainen <pmatilai@redhat.com>
Date: Wed, 10 Feb 2021 08:25:28 +0200
Subject: [PATCH 2/9] Drop unused filename variable
---
lib/fsm.c | 2 --
1 file changed, 2 deletions(-)
diff --git a/lib/fsm.c b/lib/fsm.c
index 7c291adb02..41b6267ddc 100644
--- a/lib/fsm.c
+++ b/lib/fsm.c
@@ -959,11 +959,9 @@ int rpmPackageFilesInstall(rpmts ts, rpmte te, rpmfiles files,
/* 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) {
From dcb5791066afd5caa1003a5d35903a7f5dc79cc2 Mon Sep 17 00:00:00 2001
From: Panu Matilainen <pmatilai@redhat.com>
Date: Wed, 10 Feb 2021 09:57:17 +0200
Subject: [PATCH 3/9] 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 41b6267ddc..c581a918a5 100644
--- a/lib/fsm.c
+++ b/lib/fsm.c
@@ -773,14 +773,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;
}
}
From 7fdf248e7d29a244b97c46b19be0df64992fdfae Mon Sep 17 00:00:00 2001
From: Panu Matilainen <pmatilai@redhat.com>
Date: Wed, 10 Feb 2021 09:47:19 +0200
Subject: [PATCH 4/9] 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 c581a918a5..9dba30560f 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
@@ -840,19 +848,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;
@@ -866,96 +871,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. */
@@ -967,10 +975,10 @@ int rpmPackageFilesInstall(rpmts ts, rpmte te, rpmfiles files,
}
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;
}
@@ -978,23 +986,22 @@ int rpmPackageFilesInstall(rpmts ts, rpmte te, rpmfiles files,
/* 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)
@@ -1010,7 +1017,9 @@ int rpmPackageFilesInstall(rpmts ts, rpmte te, rpmfiles files,
rpmfiFree(fi);
Fclose(payload);
free(tid);
- free(fpath);
+ for (int i = 0; i < fc; i++)
+ free(fdata[i].fpath);
+ free(fdata);
return rc;
}
@@ -1022,29 +1031,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.
@@ -1069,20 +1080,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. */
@@ -1090,10 +1101,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;
From 202c9e9cd2e199b7e7c9655704a98ab2d4fad69c Mon Sep 17 00:00:00 2001
From: Panu Matilainen <pmatilai@redhat.com>
Date: Wed, 10 Feb 2021 10:08:27 +0200
Subject: [PATCH 5/9] 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 9dba30560f..80ca234b1e 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 @@ static int expandRegular(rpmfi fi, const char *dest, rpmpsm psm, int nodigest)
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;
@@ -874,6 +875,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 {
@@ -900,8 +902,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);
@@ -927,8 +927,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) {
@@ -961,7 +961,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);
}
From f014fc46325efe92b79841145f8dc0cb40896c64 Mon Sep 17 00:00:00 2001
From: Panu Matilainen <pmatilai@redhat.com>
Date: Wed, 10 Feb 2021 10:24:22 +0200
Subject: [PATCH 6/9] 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 80ca234b1e..554ea712f5 100644
--- a/lib/fsm.c
+++ b/lib/fsm.c
@@ -876,11 +876,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 7ce1712323..e0adbd8aff 100644
--- a/lib/rpmfiles.h
+++ b/lib/rpmfiles.h
@@ -121,6 +121,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.
*/
From fc54439f8a10901cd72028cdbdb924e3e7501624 Mon Sep 17 00:00:00 2001
From: Panu Matilainen <pmatilai@redhat.com>
Date: Wed, 10 Feb 2021 10:24:22 +0200
Subject: [PATCH 7/9] 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 554ea712f5..094f5e2bb6 100644
--- a/lib/fsm.c
+++ b/lib/fsm.c
@@ -876,8 +876,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. */
From 9294ffa898bc7f1f77ff80c0a3f4f0f70c4dfef2 Mon Sep 17 00:00:00 2001
From: Panu Matilainen <pmatilai@redhat.com>
Date: Wed, 10 Feb 2021 11:25:10 +0200
Subject: [PATCH 8/9] 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 094f5e2bb6..f86383a986 100644
--- a/lib/fsm.c
+++ b/lib/fsm.c
@@ -282,24 +282,22 @@ static int expandRegular(rpmfi fi, const char *dest, rpmpsm psm, int nodigest)
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;
}
@@ -855,10 +853,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;
@@ -925,7 +923,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) {
@@ -962,13 +960,13 @@ int rpmPackageFilesInstall(rpmts ts, rpmte te, rpmfiles files,
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) {
From d6a9a00396a89d14858c6b6e2548eca2065c1d64 Mon Sep 17 00:00:00 2001
From: Panu Matilainen <pmatilai@redhat.com>
Date: Wed, 10 Feb 2021 14:15:33 +0200
Subject: [PATCH 9/9] 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 | 32 -------
tests/rpmi.at | 92 ++++++++++++++++++++
4 files changed, 189 insertions(+), 86 deletions(-)
diff --git a/lib/fsm.c b/lib/fsm.c
index f86383a986..6efd25bddd 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;
@@ -844,10 +854,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);
@@ -858,20 +867,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)))
@@ -881,20 +887,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 */
@@ -918,7 +936,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) {
@@ -954,12 +972,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 */
@@ -969,47 +981,74 @@ int rpmPackageFilesInstall(rpmts ts, rpmte te, rpmfiles files,
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 753c174cd8..3f1437d89c 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
@@ -43,4 +44,7 @@ touch $RPM_BUILD_ROOT/teet
%files
%defattr(-,root,root)
+%if %{with owned_dir}
+%dir /foo
+%endif
/foo/*
diff --git a/tests/rpmbuild.at b/tests/rpmbuild.at
index 1f0e679c44..f0eef4eff0 100644
--- a/tests/rpmbuild.at
+++ b/tests/rpmbuild.at
@@ -253,38 +253,6 @@ drwxrwxrwx zoot zoot /j/dir
[])
AT_CLEANUP
-# ------------------------------
-# hardlink tests
-AT_SETUP([rpmbuild hardlink])
-AT_KEYWORDS([build])
-RPMDB_INIT
-AT_CHECK([
-RPMDB_INIT
-
-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])
RPMDB_INIT
diff --git a/tests/rpmi.at b/tests/rpmi.at
index 42dc52ba35..295fbc230f 100644
--- a/tests/rpmi.at
+++ b/tests/rpmi.at
@@ -722,3 +722,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
+