[Rpm-maint] [rpm-software-management/rpm] [WIP] Added fapolicyd plugin (#1475)

[Panu Matilainen]

@pmatilai requested changes on this pull request.

See above, bunch of mostly minor issues and some questions to address.

> +        return RPMRC_OK;
+    }
+
+    if (stat(state->fifo_path, &s) == -1) {
+        rpmlog(RPMLOG_DEBUG, "Stat: %s -> %s\n", state->fifo_path, strerror(errno));
+        close(fd);
+        return RPMRC_OK;
+    } else {
+
+        if (!S_ISFIFO(s.st_mode)) {
+            rpmlog(RPMLOG_DEBUG, "File: %s exists but it is not a pipe!\n", state->fifo_path);
+            close(fd);
+            return RPMRC_OK;
+        }
+
+        // we require pipe to have 0660 permissions

Rpm coding style is `/* ... */` comments, please adjust for this everywhere.

> +    fd = open(state->fifo_path, O_WRONLY);
+    if(fd == -1) {
+        rpmlog(RPMLOG_DEBUG, "Open: %s -> %s\n", state->fifo_path, strerror(errno));
+        return RPMRC_OK;
+    }
+
+    if (stat(state->fifo_path, &s) == -1) {
+        rpmlog(RPMLOG_DEBUG, "Stat: %s -> %s\n", state->fifo_path, strerror(errno));
+        close(fd);
+        return RPMRC_OK;
+    } else {
+
+        if (!S_ISFIFO(s.st_mode)) {
+            rpmlog(RPMLOG_DEBUG, "File: %s exists but it is not a pipe!\n", state->fifo_path);
+            close(fd);
+            return RPMRC_OK;

There are multiple paths closing the fd and returning from this function, this is prone to introducing resource leaks sooner or later. Please refactor to use a common cleanup path for these, typically "goto errxit" style is use within rpm.

> +            !(s.st_mode & S_IWOTH) &&
+            !(s.st_mode & S_IXOTH) )) {
+
+            rpmlog(RPMLOG_ERR, "File: %s has 0%d%d%d instead of 0660 \n",
+                    state->fifo_path,
+                    ((s.st_mode & S_IRUSR) ? 4 : 0) +
+                    ((s.st_mode & S_IWUSR) ? 2 : 0) +
+                    ((s.st_mode & S_IXUSR) ? 1 : 0),
+
+                    ((s.st_mode & S_IRGRP) ? 4 : 0) +
+                    ((s.st_mode & S_IWGRP) ? 2 : 0) +
+                    ((s.st_mode & S_IXGRP) ? 1 : 0),
+
+                    ((s.st_mode & S_IROTH) ? 4 : 0) +
+                    ((s.st_mode & S_IWOTH) ? 2 : 0) +
+                    ((s.st_mode & S_IXOTH) ? 1 : 0) );

This seems rather excessive for what is just a diagnostic message. Just log the numeric s.st_mode value as is...

> +            return RPMRC_OK;
+        }
+    }
+
+    state->fd = fd;
+    state->ready = 1;
+    return RPMRC_OK;
+}
+
+static rpmRC write_fifo(struct fapolicyd_data* state, const char * str, size_t len)
+{
+    ssize_t ret = write(state->fd, str, len);
+
+    if (ret == -1) {
+        rpmlog(RPMLOG_DEBUG, "Write: %s -> %s\n", state->fifo_path, strerror(errno));
+    }

write() can fail with -EINTR, in which case you should just try again.

> +
+    /* Ignore skipped files and unowned directories */
+    if (XFA_SKIPPING(action) || (op & FAF_UNOWNED)) {
+        rpmlog(RPMLOG_DEBUG, "fapolicyd skipping early: path %s dest %s\n",
+               path, dest);
+        return RPMRC_OK;
+    }
+
+    if (!S_ISREG(rpmfiFMode(fi))) {
+        rpmlog(RPMLOG_DEBUG, "fapolicyd skipping non regular: path %s dest %s\n",
+               path, dest);
+        return RPMRC_OK;
+    }
+
+    if (!fapolicyd_state.ready)
+        return RPMRC_OK;

Here too, multiple early returns. It doesn't allocate any resources now, but if/when it will in the future a simple change becomes more painful than it should because it needs to be handled in multiple places. Please use a `goto exit` idiom to jump out instead.

> +    return RPMRC_OK;
+}
+
+static rpmRC fapolicyd_init(rpmPlugin plugin, rpmts ts)
+{
+    if (rpmtsFlags(ts) & (RPMTRANS_FLAG_TEST))
+        return RPMRC_OK;
+
+    open_fifo(&fapolicyd_state);
+    return RPMRC_OK;
+}
+
+static void fapolicyd_cleanup(rpmPlugin plugin)
+{
+    if (fapolicyd_state.ready)
+        close(fapolicyd_state.fd);

Do conditionalize the close() on the fd itself, this ensures it gets closed no matter what. Or just leave the condition away, this is just a cleanup whose return we're not checking anyhow so no harm done calling close(-1).

> +
+static rpmRC fapolicyd_tsm_post(rpmPlugin plugin, rpmts ts, int res)
+{
+    if (fapolicyd_state.ready)
+        write_fifo(&fapolicyd_state, "1", 2);
+
+    return RPMRC_OK;
+}
+
+static rpmRC fapolicyd_scriptlet_pre(rpmPlugin plugin, const char *s_name,
+				    int type)
+{
+    if (fapolicyd_state.ready && fapolicyd_state.first_scriptlet) {
+        // send signal to flush cache
+        write_fifo(&fapolicyd_state, "2", 2);
+        fapolicyd_state.first_scriptlet = 0;

Hmm, doesn't this need to take place once per package, rather than once per transaction? (I could be misunderstanding and/or misremembering the discussion though)

> +    fd = open(state->fifo_path, O_WRONLY);
+    if(fd == -1) {
+        rpmlog(RPMLOG_DEBUG, "Open: %s -> %s\n", state->fifo_path, strerror(errno));
+        return RPMRC_OK;
+    }
+
+    if (stat(state->fifo_path, &s) == -1) {
+        rpmlog(RPMLOG_DEBUG, "Stat: %s -> %s\n", state->fifo_path, strerror(errno));
+        close(fd);
+        return RPMRC_OK;
+    } else {
+
+        if (!S_ISFIFO(s.st_mode)) {
+            rpmlog(RPMLOG_DEBUG, "File: %s exists but it is not a pipe!\n", state->fifo_path);
+            close(fd);
+            return RPMRC_OK;

Another thing, more of a question: AIUI fapolicyd can actually prevent upgrades from taking place, so if it's actually active on the system then shouldn't a failure to initialize actually be returned as a failure here - its far, far better to fail early than in the middle of transaction.

> +            !(s.st_mode & S_IWOTH) &&
+            !(s.st_mode & S_IXOTH) )) {
+
+            rpmlog(RPMLOG_ERR, "File: %s has 0%d%d%d instead of 0660 \n",
+                    state->fifo_path,
+                    ((s.st_mode & S_IRUSR) ? 4 : 0) +
+                    ((s.st_mode & S_IWUSR) ? 2 : 0) +
+                    ((s.st_mode & S_IXUSR) ? 1 : 0),
+
+                    ((s.st_mode & S_IRGRP) ? 4 : 0) +
+                    ((s.st_mode & S_IWGRP) ? 2 : 0) +
+                    ((s.st_mode & S_IXGRP) ? 1 : 0),
+
+                    ((s.st_mode & S_IROTH) ? 4 : 0) +
+                    ((s.st_mode & S_IWOTH) ? 2 : 0) +
+                    ((s.st_mode & S_IXOTH) ? 1 : 0) );

Actually, I'd replace the whole above mode testing + reporting to just (not actually tested, but you get the idea):
```
mode_t mode = s.st_mode &  ~S_IFMT;
if (mode != 0660) {
   rpmlog(RPMLOG_ERR, "File: %s has %u permissions, 0660 expected\n", state->fifo_path, mode)
   goto err;
}
```

> +
+    /* Ignore skipped files and unowned directories */
+    if (XFA_SKIPPING(action) || (op & FAF_UNOWNED)) {
+        rpmlog(RPMLOG_DEBUG, "fapolicyd skipping early: path %s dest %s\n",
+               path, dest);
+        return RPMRC_OK;
+    }
+
+    if (!S_ISREG(rpmfiFMode(fi))) {
+        rpmlog(RPMLOG_DEBUG, "fapolicyd skipping non regular: path %s dest %s\n",
+               path, dest);
+        return RPMRC_OK;
+    }
+
+    if (!fapolicyd_state.ready)
+        return RPMRC_OK;

Also, I think this test should be the first in this function - if the plugin is not armed for action then it shouldn't be doing any extra checks and logging about them.

-- 
You are receiving this because you are subscribed to this thread.
Reply to this email directly or view it on GitHub:
https://github.com/rpm-software-management/rpm/pull/1475#pullrequestreview-567039144
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.rpm.org/pipermail/rpm-maint/attachments/20210113/3860fd29/attachment.html>