util: sanitize return values for virIdentity getters

The virIdentity getters are unusual in that they return -1 to indicate
"not found" and don't report any error. Change them to return -1 for
real errors, 0 for not found, and 1 for success.

Reviewed-by: Michal Privoznik <mprivozn@redhat.com>
Signed-off-by: Daniel P. Berrangé <berrange@redhat.com>

diff --git a/src/access/viraccessdriverpolkit.c b/src/access/viraccessdriverpolkit.c
index 75dbf8a0fa83b8aed1877d2be161fb84ae2a7672..e61ac6fa198de47b56e933c79016a2cb812ca29f 100644 (file)
--- a/src/access/viraccessdriverpolkit.c
+++ b/src/access/viraccessdriverpolkit.c
@@ -80,6 +80,7 @@ virAccessDriverPolkitGetCaller(const char *actionid,
 {
     virIdentityPtr identity = virIdentityGetCurrent();
     int ret = -1;
+    int rc;
 
     if (!identity) {
         virAccessError(VIR_ERR_ACCESS_DENIED,
@@ -88,17 +89,28 @@ virAccessDriverPolkitGetCaller(const char *actionid,
         return -1;
     }
 
-    if (virIdentityGetProcessID(identity, pid) < 0) {
+    if ((rc = virIdentityGetProcessID(identity, pid)) < 0)
+        goto cleanup;
+
+    if (rc == 0) {
         virAccessError(VIR_ERR_INTERNAL_ERROR, "%s",
                        _("No process ID available"));
         goto cleanup;
     }
-    if (virIdentityGetProcessTime(identity, startTime) < 0) {
+
+    if ((rc = virIdentityGetProcessTime(identity, startTime)) < 0)
+        goto cleanup;
+
+    if (rc == 0) {
         virAccessError(VIR_ERR_INTERNAL_ERROR, "%s",
                        _("No process start time available"));
         goto cleanup;
     }
-    if (virIdentityGetUNIXUserID(identity, uid) < 0) {
+
+    if ((rc = virIdentityGetUNIXUserID(identity, uid)) < 0)
+        goto cleanup;
+
+    if (rc == 0) {
         virAccessError(VIR_ERR_INTERNAL_ERROR, "%s",
                        _("No UNIX caller UID available"));
         goto cleanup;

diff --git a/src/admin/admin_server.c b/src/admin/admin_server.c
index 80e5a36679ed9d6175c45154d9e7b325bb9f0241..248df3f7959b5df9fbac871bd683a2427355e319 100644 (file)
--- a/src/admin/admin_server.c
+++ b/src/admin/admin_server.c
@@ -222,6 +222,7 @@ adminClientGetInfo(virNetServerClientPtr client,
     const char *attr = NULL;
     virTypedParameterPtr tmpparams = NULL;
     virIdentityPtr identity = NULL;
+    int rc;
 
     virCheckFlags(0, -1);
 
@@ -234,11 +235,12 @@ adminClientGetInfo(virNetServerClientPtr client,
                                  readonly) < 0)
         goto cleanup;
 
-    if (virIdentityGetSASLUserName(identity, &attr) < 0 ||
-        (attr &&
-         virTypedParamsAddString(&tmpparams, nparams, &maxparams,
-                                 VIR_CLIENT_INFO_SASL_USER_NAME,
-                                 attr) < 0))
+    if ((rc = virIdentityGetSASLUserName(identity, &attr)) < 0)
+        goto cleanup;
+    if (rc == 1 &&
+        virTypedParamsAddString(&tmpparams, nparams, &maxparams,
+                                VIR_CLIENT_INFO_SASL_USER_NAME,
+                                attr) < 0)
         goto cleanup;
 
     if (!virNetServerClientIsLocal(client)) {
@@ -247,48 +249,60 @@ adminClientGetInfo(virNetServerClientPtr client,
                                     sock_addr) < 0)
             goto cleanup;
 
-        if (virIdentityGetX509DName(identity, &attr) < 0 ||
-            (attr &&
-             virTypedParamsAddString(&tmpparams, nparams, &maxparams,
-                                     VIR_CLIENT_INFO_X509_DISTINGUISHED_NAME,
-                                     attr) < 0))
+        if ((rc = virIdentityGetX509DName(identity, &attr)) < 0)
+            goto cleanup;
+        if (rc == 1 &&
+            virTypedParamsAddString(&tmpparams, nparams, &maxparams,
+                                    VIR_CLIENT_INFO_X509_DISTINGUISHED_NAME,
+                                    attr) < 0)
             goto cleanup;
     } else {
         pid_t pid;
         uid_t uid;
         gid_t gid;
-        if (virIdentityGetUNIXUserID(identity, &uid) < 0 ||
+        if ((rc = virIdentityGetUNIXUserID(identity, &uid)) < 0)
+            goto cleanup;
+        if (rc == 1 &&
             virTypedParamsAddInt(&tmpparams, nparams, &maxparams,
                                  VIR_CLIENT_INFO_UNIX_USER_ID, uid) < 0)
             goto cleanup;
 
-        if (virIdentityGetUserName(identity, &attr) < 0 ||
+        if ((rc = virIdentityGetUserName(identity, &attr)) < 0)
+            goto cleanup;
+        if (rc == 1 &&
             virTypedParamsAddString(&tmpparams, nparams, &maxparams,
                                     VIR_CLIENT_INFO_UNIX_USER_NAME,
                                     attr) < 0)
             goto cleanup;
 
-        if (virIdentityGetUNIXGroupID(identity, &gid) < 0 ||
+        if ((rc = virIdentityGetUNIXGroupID(identity, &gid)) < 0)
+            goto cleanup;
+        if (rc == 1 &&
             virTypedParamsAddInt(&tmpparams, nparams, &maxparams,
                                  VIR_CLIENT_INFO_UNIX_GROUP_ID, gid) < 0)
             goto cleanup;
 
-        if (virIdentityGetGroupName(identity, &attr) < 0 ||
+        if ((rc = virIdentityGetGroupName(identity, &attr)) < 0)
+            goto cleanup;
+        if (rc == 1 &&
             virTypedParamsAddString(&tmpparams, nparams, &maxparams,
                                     VIR_CLIENT_INFO_UNIX_GROUP_NAME,
                                     attr) < 0)
             goto cleanup;
 
-        if (virIdentityGetProcessID(identity, &pid) < 0 ||
+        if ((rc = virIdentityGetProcessID(identity, &pid)) < 0)
+            goto cleanup;
+        if (rc == 1 &&
             virTypedParamsAddInt(&tmpparams, nparams, &maxparams,
                                  VIR_CLIENT_INFO_UNIX_PROCESS_ID, pid) < 0)
             goto cleanup;
     }
 
-    if (virIdentityGetSELinuxContext(identity, &attr) < 0 ||
-        (attr &&
-         virTypedParamsAddString(&tmpparams, nparams, &maxparams,
-                                VIR_CLIENT_INFO_SELINUX_CONTEXT, attr) < 0))
+    if ((rc = virIdentityGetSELinuxContext(identity, &attr)) < 0)
+        goto cleanup;
+    if (rc == 1 &&
+        virTypedParamsAddString(&tmpparams, nparams, &maxparams,
+                                VIR_CLIENT_INFO_SELINUX_CONTEXT, attr) < 0)
         goto cleanup;
 
     *params = tmpparams;

diff --git a/src/util/viridentity.c b/src/util/viridentity.c
index 55312fc0a0c140b476b51ea76dd6924f3b759c5d..964a33d339423f39913cc856fc2db29a16cc60e3 100644 (file)
--- a/src/util/viridentity.c
+++ b/src/util/viridentity.c
@@ -281,10 +281,8 @@ virIdentitySetAttr(virIdentityPtr ident,
  * with the identifying attribute @attr in @ident. If
  * @attr is not set, then it will simply be initialized
  * to NULL and considered as a successful read
- *
- * Returns 0 on success, -1 on error
  */
-static int
+static void
 virIdentityGetAttr(virIdentityPtr ident,
                    unsigned int attr,
                    const char **value)
@@ -292,20 +290,29 @@ virIdentityGetAttr(virIdentityPtr ident,
     VIR_DEBUG("ident=%p attribute=%d value=%p", ident, attr, value);
 
     *value = ident->attrs[attr];
-
-    return 0;
 }
 
 
+/*
+ * Returns: 0 if not present, 1 if present, -1 on error
+ */
 int virIdentityGetUserName(virIdentityPtr ident,
                            const char **username)
 {
-    return virIdentityGetAttr(ident,
-                              VIR_IDENTITY_ATTR_USER_NAME,
-                              username);
+    virIdentityGetAttr(ident,
+                       VIR_IDENTITY_ATTR_USER_NAME,
+                       username);
+
+    if (!*username)
+        return 0;
+
+    return 1;
 }
 
 
+/*
+ * Returns: 0 if not present, 1 if present, -1 on error
+ */
 int virIdentityGetUNIXUserID(virIdentityPtr ident,
                              uid_t *uid)
 {
@@ -313,31 +320,44 @@ int virIdentityGetUNIXUserID(virIdentityPtr ident,
     const char *userid;
 
     *uid = -1;
-    if (virIdentityGetAttr(ident,
-                           VIR_IDENTITY_ATTR_UNIX_USER_ID,
-                           &userid) < 0)
-        return -1;
-
+    virIdentityGetAttr(ident,
+                       VIR_IDENTITY_ATTR_UNIX_USER_ID,
+                       &userid);
     if (!userid)
-        return -1;
+        return 0;
 
-    if (virStrToLong_i(userid, NULL, 10, &val) < 0)
+    if (virStrToLong_i(userid, NULL, 10, &val) < 0) {
+        virReportError(VIR_ERR_INTERNAL_ERROR,
+                       _("Cannot parse user ID '%s'"), userid);
         return -1;
+    }
 
     *uid = (uid_t)val;
 
-    return 0;
+    return 1;
 }
 
+
+/*
+ * Returns: 0 if not present, 1 if present, -1 on error
+ */
 int virIdentityGetGroupName(virIdentityPtr ident,
                             const char **groupname)
 {
-    return virIdentityGetAttr(ident,
-                              VIR_IDENTITY_ATTR_GROUP_NAME,
-                              groupname);
+    virIdentityGetAttr(ident,
+                       VIR_IDENTITY_ATTR_GROUP_NAME,
+                       groupname);
+
+    if (!*groupname)
+        return 0;
+
+    return 1;
 }
 
 
+/*
+ * Returns: 0 if not present, 1 if present, -1 on error
+ */
 int virIdentityGetUNIXGroupID(virIdentityPtr ident,
                               gid_t *gid)
 {
@@ -345,23 +365,28 @@ int virIdentityGetUNIXGroupID(virIdentityPtr ident,
     const char *groupid;
 
     *gid = -1;
-    if (virIdentityGetAttr(ident,
-                           VIR_IDENTITY_ATTR_UNIX_GROUP_ID,
-                           &groupid) < 0)
-        return -1;
+    virIdentityGetAttr(ident,
+                       VIR_IDENTITY_ATTR_UNIX_GROUP_ID,
+                       &groupid);
 
     if (!groupid)
-        return -1;
+        return 0;
 
-    if (virStrToLong_i(groupid, NULL, 10, &val) < 0)
+    if (virStrToLong_i(groupid, NULL, 10, &val) < 0) {
+        virReportError(VIR_ERR_INTERNAL_ERROR,
+                       _("Cannot parse group ID '%s'"), groupid);
         return -1;
+    }
 
     *gid = (gid_t)val;
 
-    return 0;
+    return 1;
 }
 
 
+/*
+ * Returns: 0 if not present, 1 if present, -1 on error
+ */
 int virIdentityGetProcessID(virIdentityPtr ident,
                             pid_t *pid)
 {
@@ -369,66 +394,99 @@ int virIdentityGetProcessID(virIdentityPtr ident,
     const char *processid;
 
     *pid = 0;
-    if (virIdentityGetAttr(ident,
-                           VIR_IDENTITY_ATTR_PROCESS_ID,
-                           &processid) < 0)
-        return -1;
+    virIdentityGetAttr(ident,
+                       VIR_IDENTITY_ATTR_PROCESS_ID,
+                       &processid);
 
     if (!processid)
-        return -1;
+        return 0;
 
-    if (virStrToLong_ull(processid, NULL, 10, &val) < 0)
+    if (virStrToLong_ull(processid, NULL, 10, &val) < 0) {
+        virReportError(VIR_ERR_INTERNAL_ERROR,
+                       _("Cannot parse process ID '%s'"), processid);
         return -1;
+    }
 
     *pid = (pid_t)val;
 
-    return 0;
+    return 1;
 }
 
 
+/*
+ * Returns: 0 if not present, 1 if present, -1 on error
+ */
 int virIdentityGetProcessTime(virIdentityPtr ident,
                               unsigned long long *timestamp)
 {
     const char *processtime;
-    if (virIdentityGetAttr(ident,
-                           VIR_IDENTITY_ATTR_PROCESS_TIME,
-                           &processtime) < 0)
-        return -1;
+
+    *timestamp = 0;
+    virIdentityGetAttr(ident,
+                       VIR_IDENTITY_ATTR_PROCESS_TIME,
+                       &processtime);
 
     if (!processtime)
-        return -1;
+        return 0;
 
-    if (virStrToLong_ull(processtime, NULL, 10, timestamp) < 0)
+    if (virStrToLong_ull(processtime, NULL, 10, timestamp) < 0) {
+        virReportError(VIR_ERR_INTERNAL_ERROR,
+                       _("Cannot parse process time '%s'"), processtime);
         return -1;
+    }
 
-    return 0;
+    return 1;
 }
 
 
+/*
+ * Returns: 0 if not present, 1 if present, -1 on error
+ */
 int virIdentityGetSASLUserName(virIdentityPtr ident,
                                const char **username)
 {
-    return virIdentityGetAttr(ident,
-                              VIR_IDENTITY_ATTR_SASL_USER_NAME,
-                              username);
+    virIdentityGetAttr(ident,
+                       VIR_IDENTITY_ATTR_SASL_USER_NAME,
+                       username);
+
+    if (!*username)
+        return 0;
+
+    return 1;
 }
 
 
+/*
+ * Returns: 0 if not present, 1 if present, -1 on error
+ */
 int virIdentityGetX509DName(virIdentityPtr ident,
                             const char **dname)
 {
-    return virIdentityGetAttr(ident,
-                              VIR_IDENTITY_ATTR_X509_DISTINGUISHED_NAME,
-                              dname);
+    virIdentityGetAttr(ident,
+                       VIR_IDENTITY_ATTR_X509_DISTINGUISHED_NAME,
+                       dname);
+
+    if (!*dname)
+        return 0;
+
+    return 1;
 }
 
 
+/*
+ * Returns: 0 if not present, 1 if present, -1 on error
+ */
 int virIdentityGetSELinuxContext(virIdentityPtr ident,
                                  const char **context)
 {
-    return virIdentityGetAttr(ident,
-                              VIR_IDENTITY_ATTR_SELINUX_CONTEXT,
-                              context);
+    virIdentityGetAttr(ident,
+                       VIR_IDENTITY_ATTR_SELINUX_CONTEXT,
+                       context);
+
+    if (!*context)
+        return 0;
+
+    return 1;
 }
 
 

diff --git a/tests/viridentitytest.c b/tests/viridentitytest.c
index d76c779dd51535aee15dc99123c6a8c0c0905c4d..1eadd6173a3f1b15af5a7cba6ff3b99525a4a2e1 100644 (file)
--- a/tests/viridentitytest.c
+++ b/tests/viridentitytest.c
@@ -41,6 +41,7 @@ static int testIdentityAttrs(const void *data ATTRIBUTE_UNUSED)
     int ret = -1;
     virIdentityPtr ident;
     const char *val;
+    int rc;
 
     if (!(ident = virIdentityNew()))
         goto cleanup;
@@ -48,18 +49,18 @@ static int testIdentityAttrs(const void *data ATTRIBUTE_UNUSED)
     if (virIdentitySetUserName(ident, "fred") < 0)
         goto cleanup;
 
-    if (virIdentityGetUserName(ident, &val) < 0)
+    if ((rc = virIdentityGetUserName(ident, &val)) < 0)
         goto cleanup;
 
-    if (STRNEQ_NULLABLE(val, "fred")) {
+    if (STRNEQ_NULLABLE(val, "fred") || rc != 1) {
         VIR_DEBUG("Expected 'fred' got '%s'", NULLSTR(val));
         goto cleanup;
     }
 
-    if (virIdentityGetGroupName(ident, &val) < 0)
+    if ((rc = virIdentityGetGroupName(ident, &val)) < 0)
         goto cleanup;
 
-    if (val != NULL) {
+    if (val != NULL || rc != 0) {
         VIR_DEBUG("Unexpected groupname attribute");
         goto cleanup;
     }
@@ -69,10 +70,10 @@ static int testIdentityAttrs(const void *data ATTRIBUTE_UNUSED)
         goto cleanup;
     }
 
-    if (virIdentityGetUserName(ident, &val) < 0)
+    if ((rc = virIdentityGetUserName(ident, &val)) < 0)
         goto cleanup;
 
-    if (STRNEQ_NULLABLE(val, "fred")) {
+    if (STRNEQ_NULLABLE(val, "fred") || rc != 1) {
         VIR_DEBUG("Expected 'fred' got '%s'", NULLSTR(val));
         goto cleanup;
     }
@@ -90,6 +91,7 @@ static int testIdentityGetSystem(const void *data)
     int ret = -1;
     virIdentityPtr ident = NULL;
     const char *val;
+    int rc;
 
 #if !WITH_SELINUX
     if (context) {
@@ -104,13 +106,20 @@ static int testIdentityGetSystem(const void *data)
         goto cleanup;
     }
 
-    if (virIdentityGetSELinuxContext(ident, &val) < 0)
+    if ((rc = virIdentityGetSELinuxContext(ident, &val)) < 0)
         goto cleanup;
 
-    if (STRNEQ_NULLABLE(val, context)) {
-        VIR_DEBUG("Want SELinux context '%s' got '%s'",
-                  context, val);
-        goto cleanup;
+    if (context == NULL) {
+        if (val != NULL || rc != 0) {
+            VIR_DEBUG("Unexpected SELinux context %s", NULLSTR(val));
+            goto cleanup;
+        }
+    } else {
+        if (STRNEQ_NULLABLE(val, context) || rc != 1) {
+            VIR_DEBUG("Want SELinux context '%s' got '%s'",
+                      context, val);
+            goto cleanup;
+        }
     }
 
     ret = 0;

diff --git a/tests/virnetserverclienttest.c b/tests/virnetserverclienttest.c
index 3cd76f42ff6a0bde7c735c2e1165e1b19f7be51e..d094de9840cfaab1e7a499e8713ed2ac1522cdd8 100644 (file)
--- a/tests/virnetserverclienttest.c
+++ b/tests/virnetserverclienttest.c
@@ -85,7 +85,7 @@ static int testIdentity(const void *opaque ATTRIBUTE_UNUSED)
         goto cleanup;
     }
 
-    if (virIdentityGetUserName(ident, &gotUsername) < 0) {
+    if (virIdentityGetUserName(ident, &gotUsername) <= 0) {
         fprintf(stderr, "Missing username in identity\n");
         goto cleanup;
     }
@@ -95,7 +95,7 @@ static int testIdentity(const void *opaque ATTRIBUTE_UNUSED)
         goto cleanup;
     }
 
-    if (virIdentityGetUNIXUserID(ident, &gotUserID) < 0) {
+    if (virIdentityGetUNIXUserID(ident, &gotUserID) <= 0) {
         fprintf(stderr, "Missing user ID in identity\n");
         goto cleanup;
     }
@@ -105,7 +105,7 @@ static int testIdentity(const void *opaque ATTRIBUTE_UNUSED)
         goto cleanup;
     }
 
-    if (virIdentityGetGroupName(ident, &gotGroupname) < 0) {
+    if (virIdentityGetGroupName(ident, &gotGroupname) <= 0) {
         fprintf(stderr, "Missing groupname in identity\n");
         goto cleanup;
     }
@@ -115,7 +115,7 @@ static int testIdentity(const void *opaque ATTRIBUTE_UNUSED)
         goto cleanup;
     }
 
-    if (virIdentityGetUNIXGroupID(ident, &gotGroupID) < 0) {
+    if (virIdentityGetUNIXGroupID(ident, &gotGroupID) <= 0) {
         fprintf(stderr, "Missing group ID in identity\n");
         goto cleanup;
     }
@@ -125,7 +125,7 @@ static int testIdentity(const void *opaque ATTRIBUTE_UNUSED)
         goto cleanup;
     }
 
-    if (virIdentityGetSELinuxContext(ident, &gotSELinuxContext) < 0) {
+    if (virIdentityGetSELinuxContext(ident, &gotSELinuxContext) <= 0) {
         fprintf(stderr, "Missing SELinux context in identity\n");
         goto cleanup;
     }