[libvirt] [PATCHv4 05/15] util: use SCM_RIGHTS in virFileOperation when needed
Laine Stump
laine at laine.org
Fri Mar 11 15:34:22 UTC 2011
On 03/09/2011 08:45 PM, Eric Blake wrote:
> Currently, the hook function in virFileOperation is extremely limited:
> it must be async-signal-safe, and cannot modify any memory in the
> parent process. It is much handier to return a valid fd and operate
> on it in the parent than to deal with hook restrictions.
>
> * src/util/util.h (VIR_FILE_OP_RETURN_FD): New flag.
> * src/util/util.c (virFileOperationNoFork, virFileOperation):
> Honor new flag.
> ---
> src/util/util.c | 139 ++++++++++++++++++++++++++++++++++++++++++++++++------
> src/util/util.h | 4 +-
> 2 files changed, 126 insertions(+), 17 deletions(-)
>
> diff --git a/src/util/util.c b/src/util/util.c
> index f41e117..6bf3219 100644
> --- a/src/util/util.c
> +++ b/src/util/util.c
> @@ -1392,14 +1392,15 @@ static int virFileOperationNoFork(const char *path, int openflags, mode_t mode,
> if ((hook)&& ((ret = hook(fd, hookdata)) != 0)) {
> goto error;
> }
> + if (flags& VIR_FILE_OP_RETURN_FD)
> + return fd;
> if (VIR_CLOSE(fd)< 0) {
> ret = -errno;
> virReportSystemError(errno, _("failed to close new file '%s'"),
> path);
> - fd = -1;
> goto error;
> }
> - fd = -1;
> +
> error:
> VIR_FORCE_CLOSE(fd);
> return ret;
> @@ -1444,7 +1445,32 @@ error:
> return ret;
> }
>
> -/* return -errno on failure, or 0 on success */
> +/**
> + * virFileOperation:
> + * @path: file to open or create
> + * @openflags: flags to pass to open
> + * @mode: mode to use on creation or when forcing permissions
> + * @uid: uid that should own file on creation
> + * @gid: gid that should own file
> + * @hook: callback to run once file is open; see below for restrictions
> + * @hookdata: opaque data for @hook
> + * @flags: bit-wise or of VIR_FILE_OP_* flags
> + *
> + * Open @path, and execute an optional callback @hook on the open file
> + * description. @hook must return 0 on success, or -errno on failure.
> + * If @flags includes VIR_FILE_OP_AS_UID, then open the file while the
> + * effective user id is @uid (by using a child process); this allows
> + * one to bypass root-squashing NFS issues. If @flags includes
> + * VIR_FILE_OP_FORCE_PERMS, then ensure that @path has those
> + * permissions before the callback, even if the file already existed
> + * with different permissions. If @flags includes
> + * VIR_FILE_OP_RETURN_FD, then use SCM_RIGHTS to guarantee that any
> + * @hook is run in the parent, and the return value (if non-negative)
> + * is the file descriptor, left open. Otherwise, @hook might be run
> + * in a child process, so it must be async-signal-safe (ie. no
> + * malloc() or anything else that depends on modifying locks held in
> + * the parent), no file descriptor remains open, and 0 is returned on
> + * success. Return -errno on failure. */
> int virFileOperation(const char *path, int openflags, mode_t mode,
> uid_t uid, gid_t gid,
> virFileOperationHook hook, void *hookdata,
> @@ -1452,7 +1478,14 @@ int virFileOperation(const char *path, int openflags, mode_t mode,
> struct stat st;
> pid_t pid;
> int waitret, status, ret = 0;
> - int fd;
> + int fd = -1;
> + int pair[2] = { -1, -1 };
> + struct msghdr msg;
> + struct cmsghdr *cmsg;
> + char buf[CMSG_SPACE(sizeof(fd))];
> + char dummy = 0;
> + struct iovec iov;
> + int forkRet;
>
> if ((!(flags& VIR_FILE_OP_AS_UID))
> || (getuid() != 0)
> @@ -1466,7 +1499,29 @@ int virFileOperation(const char *path, int openflags, mode_t mode,
> * following dance avoids problems caused by root-squashing
> * NFS servers. */
>
> - int forkRet = virFork(&pid);
> + if (flags& VIR_FILE_OP_RETURN_FD) {
> + if (socketpair(AF_UNIX, SOCK_DGRAM, 0, pair)< 0) {
As mentioned in other mail, if you change SOCK_DGRAM to SOCK_STREAM, the
parent will no longer hang on recvmsg() if the child neglects to call
sendmsg().
> + ret = -errno;
> + virReportSystemError(errno,
> + _("failed to create socket needed for '%s'"),
> + path);
> + return ret;
> + }
> +
> + memset(&msg, 0, sizeof(msg));
> + iov.iov_base =&dummy;
> + iov.iov_len = 1;
> + msg.msg_iov =&iov;
> + msg.msg_iovlen = 1;
> + msg.msg_control = buf;
> + msg.msg_controllen = sizeof(buf);
> + cmsg = CMSG_FIRSTHDR(&msg);
> + cmsg->cmsg_level = SOL_SOCKET;
> + cmsg->cmsg_type = SCM_RIGHTS;
> + cmsg->cmsg_len = CMSG_LEN(sizeof(fd));
> + }
> +
> + forkRet = virFork(&pid);
>
> if (pid< 0) {
> ret = -errno;
> @@ -1474,6 +1529,31 @@ int virFileOperation(const char *path, int openflags, mode_t mode,
> }
>
> if (pid) { /* parent */
> + if (flags& VIR_FILE_OP_RETURN_FD) {
> + VIR_FORCE_CLOSE(pair[1]);
> +
> + do {
> + ret = recvmsg(pair[0],&msg, 0);
> + } while (ret< 0&& errno == EINTR);
> +
> + if (ret< 0) {
> + ret = -errno;
> + VIR_FORCE_CLOSE(pair[0]);
> + while ((waitret = waitpid(pid, NULL, 0) == -1)
> +&& (errno == EINTR));
> + goto parenterror;
> + }
> + VIR_FORCE_CLOSE(pair[0]);
> +
> + /* See if fd was transferred. */
> + cmsg = CMSG_FIRSTHDR(&msg);
> + if (cmsg&& cmsg->cmsg_len == CMSG_LEN(sizeof(fd))&&
> + cmsg->cmsg_level == SOL_SOCKET&&
> + cmsg->cmsg_type == SCM_RIGHTS) {
> + memcpy(&fd, CMSG_DATA(cmsg), sizeof(fd));
> + }
> + }
> +
> /* wait for child to complete, and retrieve its exit code */
> while ((waitret = waitpid(pid,&status, 0) == -1)
> && (errno == EINTR));
> @@ -1485,12 +1565,20 @@ int virFileOperation(const char *path, int openflags, mode_t mode,
> goto parenterror;
> }
> ret = -WEXITSTATUS(status);
> - if (!WIFEXITED(status) || (ret == -EACCES)) {
> + if (!WIFEXITED(status) || (ret == -EACCES) ||
> + ((flags& VIR_FILE_OP_RETURN_FD)&& fd == -1)) {
> /* fall back to the simpler method, which works better in
> * some cases */
> return virFileOperationNoFork(path, openflags, mode, uid, gid,
> hook, hookdata, flags);
> }
> + if (!ret&& (flags& VIR_FILE_OP_RETURN_FD)) {
> + if ((hook)&& ((ret = hook(fd, hookdata)) != 0)) {
> + VIR_FORCE_CLOSE(fd);
> + goto parenterror;
> + }
> + ret = fd;
> + }
> parenterror:
> return ret;
> }
> @@ -1500,6 +1588,7 @@ parenterror:
>
> if (forkRet< 0) {
> /* error encountered and logged in virFork() after the fork. */
> + ret = -errno;
> goto childerror;
> }
>
> @@ -1549,15 +1638,33 @@ parenterror:
> path, mode);
> goto childerror;
> }
> - if ((hook)&& ((ret = hook(fd, hookdata)) != 0)) {
> - goto childerror;
> - }
> - if (VIR_CLOSE(fd)< 0) {
> - ret = -errno;
> - virReportSystemError(errno, _("child failed to close new file '%s'"),
> - path);
> - goto childerror;
> + if (flags& VIR_FILE_OP_RETURN_FD) {
> + VIR_FORCE_CLOSE(pair[0]);
> + memcpy(CMSG_DATA(cmsg),&fd, sizeof(fd));
> +
> + do {
> + ret = sendmsg(pair[1],&msg, 0);
> + } while (ret< 0&& errno == EINTR);
> +
> + if (ret< 0) {
> + ret = -errno;
> + VIR_FORCE_CLOSE(pair[1]);
Why not just always close pair[1]? (right now you're depending on
_exit() to close it)
> + goto childerror;
> + }
> + ret = 0;
> + } else {
> + if ((hook)&& ((ret = hook(fd, hookdata)) != 0))
> + goto childerror;
> + if (VIR_CLOSE(fd)< 0) {
> + ret = -errno;
> + virReportSystemError(errno,
> + _("child failed to close new file '%s'"),
> + path);
> + goto childerror;
> + }
> }
> +
> + ret = 0;
> childerror:
> /* ret tracks -errno on failure, but exit value must be positive.
> * If the child exits with EACCES, then the parent tries again. */
> @@ -1688,7 +1795,7 @@ int virFileOperation(const char *path ATTRIBUTE_UNUSED,
> virUtilError(VIR_ERR_INTERNAL_ERROR,
> "%s", _("virFileOperation is not implemented for WIN32"));
>
> - return -1;
> + return -ENOSYS;
> }
>
> int virDirCreate(const char *path ATTRIBUTE_UNUSED,
> @@ -1700,7 +1807,7 @@ int virDirCreate(const char *path ATTRIBUTE_UNUSED,
> virUtilError(VIR_ERR_INTERNAL_ERROR,
> "%s", _("virDirCreate is not implemented for WIN32"));
>
> - return -1;
> + return -ENOSYS;
> }
> #endif /* WIN32 */
>
> diff --git a/src/util/util.h b/src/util/util.h
> index 5f6473c..3970d58 100644
> --- a/src/util/util.h
> +++ b/src/util/util.h
> @@ -128,12 +128,14 @@ enum {
> VIR_FILE_OP_NONE = 0,
> VIR_FILE_OP_AS_UID = (1<< 0),
> VIR_FILE_OP_FORCE_PERMS = (1<< 1),
> + VIR_FILE_OP_RETURN_FD = (1<< 2),
> };
> typedef int (*virFileOperationHook)(int fd, void *data);
> int virFileOperation(const char *path, int openflags, mode_t mode,
> uid_t uid, gid_t gid,
> virFileOperationHook hook, void *hookdata,
> - unsigned int flags) ATTRIBUTE_RETURN_CHECK;
> + unsigned int flags)
> + ATTRIBUTE_NONNULL(1) ATTRIBUTE_RETURN_CHECK;
>
> enum {
> VIR_DIR_CREATE_NONE = 0,
ACK with the above two nits fixed.
More information about the libvir-list
mailing list