[libvirt] [PATCH 6/9] 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. Signed-off-by: Daniel P. Berrangé <berrange(a)redhat.com&gt; --- 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