[libvirt] [PATCH 1/2] daemon: convert virRun to virCommand
Using snprintf to build up argv seems archaic. * daemon/remote.c (remoteDispatchAuthPolkit): Modernize command call. --- daemon/remote.c | 42 +++++++++++++++++------------------------- 1 files changed, 17 insertions(+), 25 deletions(-) diff --git a/daemon/remote.c b/daemon/remote.c index b158b8b..7f552a7 100644 --- a/daemon/remote.c +++ b/daemon/remote.c @@ -2468,26 +2468,17 @@ remoteDispatchAuthPolkit(virNetServerPtr server ATTRIBUTE_UNUSED, uid_t callerUid = -1; const char *action; int status = -1; - char pidbuf[50]; - char ident[100]; - int rv = -1; + char *ident = NULL; struct daemonClientPrivate *priv = virNetServerClientGetPrivateData(client); - - memset(ident, 0, sizeof ident); + virCommandPtr cmd = NULL; virMutexLock(&priv->lock); action = virNetServerClientGetReadonly(client) ? "org.libvirt.unix.monitor" : "org.libvirt.unix.manage"; - const char * const pkcheck [] = { - PKCHECK_PATH, - "--action-id", action, - "--process", pidbuf, - "--allow-user-interaction", - NULL - }; + cmd = virCommandNewArgList(PKCHECK_PATH, "--action-id", action, NULL); VIR_DEBUG("Start PolicyKit auth %d", virNetServerClientGetFD(client)); if (virNetServerClientGetAuth(client) != VIR_NET_SERVER_SERVICE_AUTH_POLKIT) { @@ -2495,28 +2486,25 @@ remoteDispatchAuthPolkit(virNetServerPtr server ATTRIBUTE_UNUSED, goto authfail; } - if (virNetServerClientGetUNIXIdentity(client, &callerUid, &callerGid, &callerPid) < 0) { + if (virNetServerClientGetUNIXIdentity(client, &callerUid, &callerGid, + &callerPid) < 0) { goto authfail; } VIR_INFO("Checking PID %d running as %d", callerPid, callerUid); - rv = snprintf(pidbuf, sizeof pidbuf, "%d", callerPid); - if (rv < 0 || rv >= sizeof pidbuf) { - VIR_ERROR(_("Caller PID was too large %d"), callerPid); - goto authfail; - } + virCommandAddArg(cmd, "--process"); + virCommandAddArgFormat(cmd, "%d", callerPid); + virCommandAddArg(cmd, "--allow-user-interaction"); - rv = snprintf(ident, sizeof ident, "pid:%d,uid:%d", callerPid, callerUid); - if (rv < 0 || rv >= sizeof ident) { - VIR_ERROR(_("Caller identity was too large %d:%d"), callerPid, callerUid); + if (virAsprintf(&ident, "pid:%d,uid:%d", callerPid, callerUid) < 0) { + virReportOOMError(); goto authfail; } - if (virRun(pkcheck, &status) < 0) { - VIR_ERROR(_("Cannot invoke %s"), PKCHECK_PATH); + if (virCommandRun(cmd, &status) < 0) goto authfail; - } + if (status != 0) { char *tmp = virCommandTranslateStatus(status); VIR_ERROR(_("Policy kit denied action %s from pid %d, uid %d: %s"), @@ -2533,10 +2521,14 @@ remoteDispatchAuthPolkit(virNetServerPtr server ATTRIBUTE_UNUSED, virNetServerClientSetIdentity(client, ident); virMutexUnlock(&priv->lock); + virCommandFree(cmd); + VIR_FREE(ident); return 0; error: + virCommandFree(cmd); + VIR_FREE(ident); virResetLastError(); virNetError(VIR_ERR_AUTH_FAILED, "%s", _("authentication failed")); @@ -2553,7 +2545,7 @@ authfail: authdeny: PROBE(RPC_SERVER_CLIENT_AUTH_DENY, "client=%p auth=%d identity=%s", - client, REMOTE_AUTH_POLKIT, (char *)ident); + client, REMOTE_AUTH_POLKIT, ident); goto error; } #elif HAVE_POLKIT0 -- 1.7.7.6
Convert daemon code to handle 64-bit pid_t (even though at the moment, it is not compiled on mingw). * daemon/remote.c (remoteDispatchAuthList) (remoteDispatchAuthPolkit): Print pid_t via %lld. --- daemon/remote.c | 50 +++++++++++++++++++++++++++++--------------------- 1 files changed, 29 insertions(+), 21 deletions(-) diff --git a/daemon/remote.c b/daemon/remote.c index 7f552a7..1ada146 100644 --- a/daemon/remote.c +++ b/daemon/remote.c @@ -2039,20 +2039,22 @@ remoteDispatchAuthList(virNetServerPtr server ATTRIBUTE_UNUSED, * some piece of polkit isn't present/running */ if (auth == VIR_NET_SERVER_SERVICE_AUTH_POLKIT) { - if (virNetServerClientGetUNIXIdentity(client, &callerUid, &callerGid, &callerPid) < 0) { + if (virNetServerClientGetUNIXIdentity(client, &callerUid, &callerGid, + &callerPid) < 0) { /* Don't do anything on error - it'll be validated at next * phase of auth anyway */ virResetLastError(); } else if (callerUid == 0) { - char ident[100]; - rv = snprintf(ident, sizeof ident, "pid:%d,uid:%d", callerPid, callerUid); - if (rv > 0 || rv < sizeof ident) { + char *ident; + if (virAsprintf(&ident, "pid:%lld,uid:%d", + (long long) callerPid, callerUid) == 0) { VIR_INFO("Bypass polkit auth for privileged client %s", ident); if (virNetServerClientSetIdentity(client, ident) < 0) virResetLastError(); else auth = VIR_NET_SERVER_SERVICE_AUTH_NONE; + VIR_FREE(ident); } rv = -1; } @@ -2491,13 +2493,15 @@ remoteDispatchAuthPolkit(virNetServerPtr server ATTRIBUTE_UNUSED, goto authfail; } - VIR_INFO("Checking PID %d running as %d", callerPid, callerUid); + VIR_INFO("Checking PID %lld running as %d", + (long long) callerPid, callerUid); virCommandAddArg(cmd, "--process"); - virCommandAddArgFormat(cmd, "%d", callerPid); + virCommandAddArgFormat(cmd, "%lld", (long long) callerPid); virCommandAddArg(cmd, "--allow-user-interaction"); - if (virAsprintf(&ident, "pid:%d,uid:%d", callerPid, callerUid) < 0) { + if (virAsprintf(&ident, "pid:%lld,uid:%d", + (long long) callerPid, callerUid) < 0) { virReportOOMError(); goto authfail; } @@ -2507,16 +2511,16 @@ remoteDispatchAuthPolkit(virNetServerPtr server ATTRIBUTE_UNUSED, if (status != 0) { char *tmp = virCommandTranslateStatus(status); - VIR_ERROR(_("Policy kit denied action %s from pid %d, uid %d: %s"), - action, callerPid, callerUid, NULLSTR(tmp)); + VIR_ERROR(_("Policy kit denied action %s from pid %lld, uid %d: %s"), + action, (long long) callerPid, callerUid, NULLSTR(tmp)); VIR_FREE(tmp); goto authdeny; } PROBE(RPC_SERVER_CLIENT_AUTH_ALLOW, "client=%p auth=%d identity=%s", client, REMOTE_AUTH_POLKIT, ident); - VIR_INFO("Policy allowed action %s from pid %d, uid %d", - action, callerPid, callerUid); + VIR_INFO("Policy allowed action %s from pid %lld, uid %d", + action, (long long) callerPid, callerUid); ret->complete = 1; virNetServerClientSetIdentity(client, ident); @@ -2566,7 +2570,7 @@ remoteDispatchAuthPolkit(virNetServerPtr server, PolKitResult pkresult; DBusError err; const char *action; - char ident[100]; + char *ident = NULL; int rv = -1; struct daemonClientPrivate *priv = virNetServerClientGetPrivateData(client); @@ -2585,18 +2589,20 @@ remoteDispatchAuthPolkit(virNetServerPtr server, goto authfail; } - if (virNetServerClientGetUNIXIdentity(client, &callerUid, &callerGid, &callerPid) < 0) { + if (virNetServerClientGetUNIXIdentity(client, &callerUid, &callerGid, + &callerPid) < 0) { VIR_ERROR(_("cannot get peer socket identity")); goto authfail; } - rv = snprintf(ident, sizeof ident, "pid:%d,uid:%d", callerPid, callerUid); - if (rv < 0 || rv >= sizeof ident) { - VIR_ERROR(_("Caller identity was too large %d:%d"), callerPid, callerUid); + if (virAsprintf(&ident, "pid:%lld,uid:%d", + (long long) callerPid, callerUid) < 0) { + virReportOOMError(); goto authfail; } - VIR_INFO("Checking PID %d running as %d", callerPid, callerUid); + VIR_INFO("Checking PID %lld running as %d", + (long long) callerPid, callerUid); dbus_error_init(&err); if (!(pkcaller = polkit_caller_new_from_pid(virNetServerGetDBusConn(server), callerPid, &err))) { @@ -2649,24 +2655,26 @@ remoteDispatchAuthPolkit(virNetServerPtr server, polkit_caller_unref(pkcaller); polkit_action_unref(pkaction); if (pkresult != POLKIT_RESULT_YES) { - VIR_ERROR(_("Policy kit denied action %s from pid %d, uid %d, result: %s"), - action, callerPid, callerUid, + VIR_ERROR(_("Policy kit denied action %s from pid %lld, uid %d, result: %s"), + action, (long long) callerPid, callerUid, polkit_result_to_string_representation(pkresult)); goto authdeny; } PROBE(RPC_SERVER_CLIENT_AUTH_ALLOW, "client=%p auth=%d identity=%s", client, REMOTE_AUTH_POLKIT, ident); - VIR_INFO("Policy allowed action %s from pid %d, uid %d, result %s", - action, callerPid, callerUid, + VIR_INFO("Policy allowed action %s from pid %lld, uid %d, result %s", + action, (long long) callerPid, callerUid, polkit_result_to_string_representation(pkresult)); ret->complete = 1; virNetServerClientSetIdentity(client, ident); virMutexUnlock(&priv->lock); + VIR_FREE(ident); return 0; error: + VIR_FREE(ident); virResetLastError(); virNetError(VIR_ERR_AUTH_FAILED, "%s", _("authentication failed")); -- 1.7.7.6
On 26.01.2012 01:51, Eric Blake wrote:
Convert daemon code to handle 64-bit pid_t (even though at the moment, it is not compiled on mingw).
* daemon/remote.c (remoteDispatchAuthList) (remoteDispatchAuthPolkit): Print pid_t via %lld. --- daemon/remote.c | 50 +++++++++++++++++++++++++++++--------------------- 1 files changed, 29 insertions(+), 21 deletions(-)
ACK, although I haven't seen an architecture with 8B pid_t. But reading POSIX pid_t should be signed int. Michal
On 01/26/2012 02:08 AM, Michal Privoznik wrote:
On 26.01.2012 01:51, Eric Blake wrote:
Convert daemon code to handle 64-bit pid_t (even though at the moment, it is not compiled on mingw).
* daemon/remote.c (remoteDispatchAuthList) (remoteDispatchAuthPolkit): Print pid_t via %lld. --- daemon/remote.c | 50 +++++++++++++++++++++++++++++--------------------- 1 files changed, 29 insertions(+), 21 deletions(-)
ACK, although I haven't seen an architecture with 8B pid_t. But reading POSIX pid_t should be signed int.
mingw64 has 64-bit pid_t. POSIX allows that, since it only requires that pid_t be a signed integral value, not any particular size. -- Eric Blake eblake@redhat.com +1-919-301-3266 Libvirt virtualization library http://libvirt.org
On 26.01.2012 01:51, Eric Blake wrote:
Using snprintf to build up argv seems archaic.
* daemon/remote.c (remoteDispatchAuthPolkit): Modernize command call. --- daemon/remote.c | 42 +++++++++++++++++------------------------- 1 files changed, 17 insertions(+), 25 deletions(-)
ACK. I wonder if we should force developers to use virAsnprintf() instead of snprintf() like we already do for malloc(), realloc(), etc. Michal
On 01/26/2012 01:56 AM, Michal Privoznik wrote:
On 26.01.2012 01:51, Eric Blake wrote:
Using snprintf to build up argv seems archaic.
* daemon/remote.c (remoteDispatchAuthPolkit): Modernize command call. --- daemon/remote.c | 42 +++++++++++++++++------------------------- 1 files changed, 17 insertions(+), 25 deletions(-)
ACK. I wonder if we should force developers to use virAsnprintf() instead of snprintf() like we already do for malloc(), realloc(), etc.
Series pushed. There are still some valid uses of snprintf (in situations like generating a unix socket name into a fixed-width structure), so I don't think we can outright forbid it. But yes, there's quite a few places where dynamic sizing makes the code easier to write. -- Eric Blake eblake@redhat.com +1-919-301-3266 Libvirt virtualization library http://libvirt.org
participants (2)
-
Eric Blake -
Michal Privoznik