[libvirt] [PATCH 6/9] util: sanitize return values for virIdentity getters
Daniel P. Berrangé
berrange at redhat.com
Thu Sep 5 11:56:24 UTC 2019
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.
Signed-off-by: Daniel P. Berrangé <berrange at redhat.com>
---
src/access/viraccessdriverpolkit.c | 18 +++-
src/admin/admin_server.c | 52 ++++++----
src/util/viridentity.c | 156 ++++++++++++++++++++---------
tests/viridentitytest.c | 31 ++++--
tests/virnetserverclienttest.c | 10 +-
5 files changed, 180 insertions(+), 87 deletions(-)
diff --git a/src/access/viraccessdriverpolkit.c b/src/access/viraccessdriverpolkit.c
index 75dbf8a0fa..e61ac6fa19 100644
--- 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 80e5a36679..248df3f795 100644
--- 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 55312fc0a0..964a33d339 100644
--- 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 d76c779dd5..1eadd6173a 100644
--- 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 3cd76f42ff..d094de9840 100644
--- 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;
}
--
2.21.0
More information about the libvir-list
mailing list