[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]

Re: [libvirt] [PATCH v5 2/5] util: Create virFileAccessibleAs function

On 10/21/2011 07:12 AM, Michal Privoznik wrote:
This function checks if a given path is accessible under
given uid and gid.
  src/util/util.c |   83 +++++++++++++++++++++++++++++++++++++++++++++++++++++++
  src/util/util.h |    3 ++
  2 files changed, 86 insertions(+), 0 deletions(-)

diff --git a/src/util/util.c b/src/util/util.c
index af27344..7343485 100644
--- a/src/util/util.c
+++ b/src/util/util.c
@@ -671,6 +671,77 @@ virFileIsExecutable(const char *file)

  #ifndef WIN32
+/* Check that a file is accessible under certain
+ * user&  gid.
+ * @mode can be F_OK, or a bitwise combination of R_OK, W_OK, and X_OK.
+ * see 'man access' for more details.
+ * Returns 0 on success, -errno on fail,
+ *>0 on signaled child.

That's awkward.  How about just stating:

Returns 0 on success, -1 on fail with errno set.

+ */
+virFileAccessibleAs(const char *path, int mode,
+                    uid_t uid, gid_t gid)
+    pid_t pid = 0;
+    int waitret, status, ret = 0;
+    int forkRet = 0;
+    bool do_fork = (uid != getuid() || gid != getgid());
+    if (do_fork)
+        forkRet = virFork(&pid);

The control flow is a bit hard to follow. While it looks correct, I think it would be easier to read as follows (it also matches my proposal to return -1 with errno set, when encountering error):

if (uid == getuid() && gid == getgid())
    return access(path, mode);

forkRet = virFork(&pid);

... code without reference to do_fork ...

+    if (pid) { /* parent */
+        do {
+            if ((waitret = waitpid(pid,&status, 0))<  0) {
+                ret = -errno;
+                virKillProcess(pid, SIGKILL);
+                return ret;
+            }
+        } while (!WIFEXITED(status)&&  !WIFSIGNALED(status));

Use virPidWait() (from command.h), not this hand-rolled do/while loop. In particular, your loop will only ever execute exactly once, since status will always satisfy either WIFEXITED or WIFSIGNALED if waitpid() was successful; and you fail to handle EINTR as a reason to retry waitpid.

Returning a positive value for a signaled child is awkward; I would just treat that as failure, by setting errno to EIO and returning -1. The likelihood of the child dying by signal is super-slim, not to mention that I don't see how the client can behave any better by knowing that the child process died than if it just reacted as if access were denied in the first place.

@@ -993,6 +1064,18 @@ childerror:

  #else /* WIN32 */

+virFileAccessibleAs(const char *path ATTRIBUTE_UNUSED,
+                    int mode ATTRIBUTE_UNUSED,
+                    uid_t uid ATTRIBUTE_UNUSED,
+                    git_t gid ATTRIBUTE_UNUSED)
+                 "%s", _("virFileAccessibleAs is not implemented for WIN32"));
+    return -ENOSYS;

Rather than failing with -ENOSYS, can we at least try access(path, mode), ignoring uid and gid? It may have false negatives (fail in cases where a different uid would succeed), but is more likely to be useful than just blindly declaring failure.

Eric Blake   eblake redhat com    +1-801-349-2682
Libvirt virtualization library http://libvirt.org

[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]