On 02/27/2013 11:14 AM, Panu Matilainen wrote:
On 02/27/2013 08:18 AM, Reshetova, Elena wrote:
Right, no surprises: there is an issue with hard links :)
Oh, this is smth I should have actually remembered, but forgot :( I
saw the
I forgot the specifics of hardlinks too, no worries :)
issue a while back on our system setup, but since from security labelling
hard links aren't interesting (security context should be set on a file
itself, not a hard link), I simply didn't call hooks for hard links.
But I
agree that it might be important to know that they will be created and
I guess
in this case we would need to know when and where it goes. Actually
not from
labelling point of view, but from general security, a tightly configured
system might have restrictions on where files/dirs/links are created. For
example, it might not allow creating hard links in certain paths (in
order for
system to stay well arranged or for example in order to avoid untrusted
packages creating hard links in sensitive dirs). The thing here is
that such a
check should be probably performed before we start to run FSM and
unpacking
the archive (for the same reason as conflicts).
Yup, restrictions would need to be enforced early on, FSM is too late.
So, then if we consider that
labelling and policy enforcement aren't valid use cases here, we only
have
informational use case left: plugin wants to know the links and their
location
as for any other files. Should we then create separate hook/call same
hook
(with modified parameters) actually at the moment when links are created
(later on)?
Hmm. Good question... arranging pre/post-commit in fsmCommit() hooks
should be trivial. Although by now, I'm actually starting to wonder
whether it really makes sense to call any hooks for skipped files
afterall. The "informational use cases" could just go and look at the
package file list and their states from psm pre/post-hooks.
Looking at this, I just realized that rpm is currently doing chmod(),
chown() and all for each hardlink it creates, which just doesn't make
sense because ... by the very definition of a hardlink, it doesn't.
Probably worth fixing regardless of what we end up doing with hooks, eg
additional "setmeta" argument to fsmCommit() whether it should just
create or set the additional metadata as well, and have pre/post-commit
hooks get that so plugins get notified of all file creations but also
can avoid redundant setting of labels etc.
FWIW, this part is now pushed to git master, ie for hardlink sets the
metadata (permissions etc) is only set once.
I'm going to poke around with this a bit to see what would make most
sense, now that I have sufficient selinux plugin code to test it with.
Like said, I'm starting to have second thoughts on the skipped files, so
I'll probably look at changing the existing hooks to the "commit model"
rather than add more: for actually created (and removed) files, the hook
semantics would be rather obvious. With skipped (and some delayed and
whatnot) files it gets far more convoluted. If it turns out the plugins
*really* need the skipped file info as well, we can always add (back)
more hooks later on :)
Attached is one "study" into this direction, ie hooks are only called
for files that are actually acted upon. This is a total diff of multiple
commits with some semi-unrelated changes, so its more useful to look at
the resulting code more than the diff itself.
I'm not going to push this stuff until further discussion + thought, if
at all: the more I look and think about this stuff, something about it
all just doesn't feel quite right :)
Perhaps the problem is the hooks are too generic for their own good now.
One possibility (that's perhaps more clear with the other cleanup work
in the patch) is to have a "set additional file metadata" hook, as
*that* is what our current use-cases (SELinux and MSSF) want to do.
Which should actually be as simple as adding something like this after
all the fsmChown(), fsmChmod() etc calls:
if (!rc)
rc = rpmpluginsCallFsmFileMeta(fsm->plugins, fsm->path, ...)
And actually that brings it right next to where fsmSetSELabel() is
currently getting called. I've a feeling a symmetric pre/post-hook pair
doesn't actually make sense for this particular purpose, it only
complicates things unnecessarily.
Thoughts?
- Panu -
diff --git a/lib/fsm.c b/lib/fsm.c
index f28010c..aebb885 100644
--- a/lib/fsm.c
+++ b/lib/fsm.c
@@ -463,7 +463,7 @@ static int fsmMapPath(FSM_t fsm, int i)
*/
static int saveHardLink(FSM_t fsm, hardLink_t * linkSet)
{
- struct stat * st = &fsm->sb;
+ const struct stat * st = &fsm->sb;
int rc = 0;
int ix = -1;
int j;
@@ -1052,18 +1052,17 @@ static int fsmMakeLinks(FSM_t fsm, hardLink_t li)
return ec;
}
-static int fsmCommit(FSM_t fsm, int ix, int setmeta);
+static int fsmCommitCreate(FSM_t fsm, int ix, const struct stat *st, int setmeta);
/** \ingroup payload
* Commit hard linked file set atomically.
* @param fsm file state machine data
* @return 0 on success
*/
-static int fsmCommitLinks(FSM_t fsm)
+static int fsmCommitLinks(FSM_t fsm, const struct stat *st)
{
char * path = fsm->path;
const char * nsuffix = fsm->nsuffix;
- struct stat * st = &fsm->sb;
int rc = 0;
int setmeta = 1;
nlink_t i;
@@ -1081,7 +1080,7 @@ static int fsmCommitLinks(FSM_t fsm)
if (li->filex[i] < 0) continue;
rc = fsmMapPath(fsm, li->filex[i]);
if (!XFA_SKIPPING(fsm->action)) {
- rc = fsmCommit(fsm, li->filex[i], setmeta);
+ rc = fsmCommitCreate(fsm, li->filex[i], st, setmeta);
/* only the first created link needs permissions etc to be set */
if (!rc)
setmeta = 0;
@@ -1542,18 +1541,45 @@ static int fsmBackup(FSM_t fsm)
return rc;
}
-static int fsmCommit(FSM_t fsm, int ix, int setmeta)
+static int fsmSetMeta(FSM_t fsm, int ix, const struct stat *st)
+{
+ int rc = 0;
+
+ /* Set file security context (if enabled) */
+ if (!getuid()) {
+ rc = fsmSetSELabel(fsm->sehandle, fsm->path, st->st_mode);
+ }
+ if (S_ISLNK(st->st_mode)) {
+ if (!rc && !getuid())
+ rc = fsmLChown(fsm->path, st->st_uid, st->st_gid);
+ } else {
+ rpmfi fi = fsmGetFi(fsm);
+ if (!rc && !getuid())
+ rc = fsmChown(fsm->path, st->st_uid, st->st_gid);
+ if (!rc)
+ rc = fsmChmod(fsm->path, st->st_mode);
+ if (!rc) {
+ rc = fsmUtime(fsm->path, rpmfiFMtimeIndex(fi, ix));
+ /* utime error is not critical for directories */
+ if (rc && S_ISDIR(st->st_mode))
+ rc = 0;
+ }
+ /* Set file capabilities (if enabled) */
+ if (!rc && !S_ISDIR(st->st_mode) && !getuid()) {
+ rc = fsmSetFCaps(fsm->path, rpmfiFCapsIndex(fi, ix));
+ }
+ }
+ return rc;
+}
+
+static int fsmCommitCreate(FSM_t fsm, int ix, const struct stat *st, int setmeta)
{
int rc = 0;
- struct stat * st = &fsm->sb;
/* XXX Special case /dev/log, which shouldn't be packaged anyways */
if (S_ISSOCK(st->st_mode) && IS_DEV_LOG(fsm->path))
return 0;
- /* Backup on-disk file if needed. Directories are handled earlier */
- if (!S_ISDIR(st->st_mode))
- rc = fsmBackup(fsm);
/* Rename temporary to final file name. */
if (!S_ISDIR(st->st_mode) && (fsm->suffix || fsm->nsuffix)) {
char *npath = fsmFsPath(fsm, 0, fsm->nsuffix);
@@ -1568,37 +1594,90 @@ static int fsmCommit(FSM_t fsm, int ix, int setmeta)
fsm->path = npath;
}
- if (setmeta) {
- /* Set file security context (if enabled) */
- if (!rc && !getuid()) {
- rc = fsmSetSELabel(fsm->sehandle, fsm->path, fsm->sb.st_mode);
- }
- if (S_ISLNK(st->st_mode)) {
- if (!rc && !getuid())
- rc = fsmLChown(fsm->path, fsm->sb.st_uid, fsm->sb.st_gid);
- } else {
- rpmfi fi = fsmGetFi(fsm);
- if (!rc && !getuid())
- rc = fsmChown(fsm->path, fsm->sb.st_uid, fsm->sb.st_gid);
- if (!rc)
- rc = fsmChmod(fsm->path, fsm->sb.st_mode);
- if (!rc) {
- rc = fsmUtime(fsm->path, rpmfiFMtimeIndex(fi, ix));
- /* utime error is not critical for directories */
- if (rc && S_ISDIR(st->st_mode))
- rc = 0;
- }
- /* Set file capabilities (if enabled) */
- if (!rc && !S_ISDIR(st->st_mode) && !getuid()) {
- rc = fsmSetFCaps(fsm->path, rpmfiFCapsIndex(fi, ix));
- }
- }
+ if (!rc && setmeta) {
+ rc = fsmSetMeta(fsm, ix, st);
}
+ return rc;
+}
+
+static int fsmCommitErase(FSM_t fsm, const struct stat *st)
+{
+ int missingok = (fsm->fflags & (RPMFILE_MISSINGOK | RPMFILE_GHOST));
+ int rc = 0;
+
+ if (S_ISDIR(st->st_mode)) {
+ rc = fsmRmdir(fsm->path);
+ } else {
+ rc = fsmUnlink(fsm->path, fsm->mapFlags);
+ }
+
+ /*
+ * Missing %ghost or %missingok entries are not errors.
+ * XXX: Are non-existent files ever an actual error here? Afterall
+ * that's exactly what we're trying to accomplish here,
+ * and complaining about job already done seems like kinderkarten
+ * level "But it was MY turn!" whining...
+ */
+ if (rc == CPIOERR_ENOENT && missingok) {
+ rc = 0;
+ }
+
+ /*
+ * Dont whine on non-empty directories for now. We might be able
+ * to track at least some of the expected failures though,
+ * such as when we knowingly left config file backups etc behind.
+ */
+ if (rc == CPIOERR_ENOTEMPTY) {
+ rc = 0;
+ }
+
+ if (rc) {
+ int lvl = strict_erasures ? RPMLOG_ERR : RPMLOG_WARNING;
+ rpmlog(lvl, _("%s %s: remove failed: %s\n"),
+ S_ISDIR(fsm->sb.st_mode) ? _("directory") : _("file"),
+ fsm->path, strerror(errno));
+ }
+
+ return rc;
+}
+
+static int fsmCommit(FSM_t fsm, const struct stat *st)
+{
+ int rc = 0;
+
+ /* Run fsm file pre hook for all plugins */
+ rc = rpmpluginsCallFsmFilePre(fsm->plugins, fsm->path, fsm->sb.st_mode,
+ DIR_TYPE_NORMAL, fsm->action);
+
+ if (!rc) {
+ /* Backup on-disk file if needed. Directories are handled earlier */
+ if (!S_ISDIR(st->st_mode))
+ rc = fsmBackup(fsm);
+
+ if (fsm->goal == FSM_PKGINSTALL) {
+ if ((S_ISREG(st->st_mode) && st->st_nlink > 1)) {
+ rc = fsmCommitLinks(fsm, st);
+ } else {
+ rc = fsmCommitCreate(fsm, fsm->ix, st, 1);
+ }
+ } else if (fsm->goal == FSM_PKGERASE) {
+ /* Dont bother if fsmBackup() moved it already */
+ if (fsm->action == FA_ERASE) {
+ rc = fsmCommitErase(fsm, st);
+ }
+ }
+ }
+
+ /* Run fsm file post hook for all plugins */
+ rpmpluginsCallFsmFilePost(fsm->plugins, fsm->path, fsm->sb.st_mode,
+ DIR_TYPE_NORMAL, fsm->action, rc);
+
if (rc && fsm->failedFile && *fsm->failedFile == NULL) {
*fsm->failedFile = fsm->path;
fsm->path = NULL;
}
+
return rc;
}
@@ -1705,15 +1784,9 @@ int rpmPackageFilesInstall(rpmts ts, rpmte te, rpmfi fi, FD_t cfd,
if (rc)
break;
- /* Run fsm file pre hook for all plugins */
- rc = rpmpluginsCallFsmFilePre(fsm->plugins, fsm->path, fsm->sb.st_mode, DIR_TYPE_NORMAL, fsm->action);
- if (rc) {
- fsm->postpone = 1;
- } else {
- if (S_ISREG(fsm->sb.st_mode) && fsm->sb.st_nlink > 1)
- fsm->postpone = saveHardLink(fsm, &li);
- setFileState(rpmteGetFileStates(te), fsm->ix, fsm->action);
- }
+ if (S_ISREG(fsm->sb.st_mode) && fsm->sb.st_nlink > 1)
+ fsm->postpone = saveHardLink(fsm, &li);
+ setFileState(rpmteGetFileStates(te), fsm->ix, fsm->action);
if (!fsm->postpone) {
if (S_ISREG(st->st_mode)) {
@@ -1770,33 +1843,22 @@ int rpmPackageFilesInstall(rpmts ts, rpmte te, rpmfi fi, FD_t cfd,
}
}
- if (rc) {
- if (!fsm->postpone) {
- /* XXX only erase if temp fn w suffix is in use */
- if (fsm->suffix) {
- if (S_ISDIR(st->st_mode)) {
- (void) fsmRmdir(fsm->path);
- } else {
- (void) fsmUnlink(fsm->path, fsm->mapFlags);
- }
- }
- errno = saveerrno;
- if (fsm->failedFile && *fsm->failedFile == NULL)
- *fsm->failedFile = xstrdup(fsm->path);
- }
- } else {
+ if (rc == 0) {
/* Notify on success. */
rpmpsmNotify(psm, RPMCALLBACK_INST_PROGRESS, rpmcpioTell(archive));
if (!fsm->postpone) {
- rc = ((S_ISREG(st->st_mode) && st->st_nlink > 1)
- ? fsmCommitLinks(fsm) : fsmCommit(fsm, fsm->ix, 1));
+ rc = fsmCommit(fsm, st);
}
}
- /* Run fsm file post hook for all plugins */
- rpmpluginsCallFsmFilePost(fsm->plugins, fsm->path, fsm->sb.st_mode, DIR_TYPE_NORMAL, fsm->action, rc);
-
+ if (rc && !fsm->postpone) {
+ /* XXX only erase if temp fn w suffix is in use */
+ if (fsm->suffix) {
+ fsmCommitErase(fsm, st);
+ }
+ errno = saveerrno;
+ }
}
if (!rc)
@@ -1835,53 +1897,11 @@ int rpmPackageFilesRemove(rpmts ts, rpmte te, rpmfi fi,
rc = fsmInit(fsm);
- /* Run fsm file pre hook for all plugins */
- rc = rpmpluginsCallFsmFilePre(fsm->plugins, fsm->path, fsm->sb.st_mode, DIR_TYPE_NORMAL, fsm->action);
-
- if (!fsm->postpone)
- rc = fsmBackup(fsm);
-
/* Remove erased files. */
- if (!fsm->postpone && fsm->action == FA_ERASE) {
- int missingok = (fsm->fflags & (RPMFILE_MISSINGOK | RPMFILE_GHOST));
-
- if (S_ISDIR(fsm->sb.st_mode)) {
- rc = fsmRmdir(fsm->path);
- } else {
- rc = fsmUnlink(fsm->path, fsm->mapFlags);
- }
-
- /*
- * Missing %ghost or %missingok entries are not errors.
- * XXX: Are non-existent files ever an actual error here? Afterall
- * that's exactly what we're trying to accomplish here,
- * and complaining about job already done seems like kinderkarten
- * level "But it was MY turn!" whining...
- */
- if (rc == CPIOERR_ENOENT && missingok) {
- rc = 0;
- }
-
- /*
- * Dont whine on non-empty directories for now. We might be able
- * to track at least some of the expected failures though,
- * such as when we knowingly left config file backups etc behind.
- */
- if (rc == CPIOERR_ENOTEMPTY) {
- rc = 0;
- }
-
- if (rc) {
- int lvl = strict_erasures ? RPMLOG_ERR : RPMLOG_WARNING;
- rpmlog(lvl, _("%s %s: remove failed: %s\n"),
- S_ISDIR(fsm->sb.st_mode) ? _("directory") : _("file"),
- fsm->path, strerror(errno));
- }
+ if (!fsm->postpone) {
+ rc = fsmCommit(fsm, &fsm->sb);;
}
- /* Run fsm file post hook for all plugins */
- rpmpluginsCallFsmFilePost(fsm->plugins, fsm->path, fsm->sb.st_mode, DIR_TYPE_NORMAL, fsm->action, rc);
-
/* XXX Failure to remove is not (yet) cause for failure. */
if (!strict_erasures) rc = 0;
_______________________________________________
Rpm-maint mailing list
Rpm-maint@lists.rpm.org
http://lists.rpm.org/mailman/listinfo/rpm-maint
