[PATCH] HS: returns results for wrong object path
# HG changeset patch # User Heidi Eckhart <heidieck@linux.vnet.ibm.com> # Date 1199797461 -3600 # Node ID 555126035e912e9202ebcc93483ee936be5ae998 # Parent 1bbea87ad37dcb95321f43aa5e17e6fa7ec1836d HS: returns results for wrong object path The client given reference was not checked for existance in both directions (from HostSystem to Service and reverse) and the call returned wrong results. Now NOT_FOUND is returned in case of a wrong object path. Examples: wbemcli ain -ac KVM_HostedService 'http://localhost/root/virt:KVM_ResourcePoolConfigurationService.CreationClassName="wrong",wrong="wrong"' wbemcli ain -ac KVM_HostedService 'http://localhost/root/virt:KVM_HostSystem.CreationClassName="KVM_HostSystem",Name="notthere"' Signed-off-by: Heidi Eckhart <heidieck@linux.vnet.ibm.com> diff -r 1bbea87ad37d -r 555126035e91 src/Virt_HostedService.c --- a/src/Virt_HostedService.c Mon Jan 07 11:42:40 2008 -0800 +++ b/src/Virt_HostedService.c Tue Jan 08 14:04:21 2008 +0100 @@ -37,6 +37,57 @@ const static CMPIBroker *_BROKER; +static CMPIStatus check_host_ref(const CMPIObjectPath *ref) +{ + CMPIStatus s = {CMPI_RC_OK, NULL}; + CMPIInstance *inst; + const char *prop; + + s = get_host_cs(_BROKER, ref, &inst); + + prop = cu_compare_ref(ref, inst); + if (prop != NULL) { + cu_statusf(_BROKER, &s, + CMPI_RC_ERR_NOT_FOUND, + "No such instance (%s)", prop); + } + + return s; +} + +static CMPIStatus check_service_ref(const CMPIObjectPath *ref) +{ + CMPIStatus s = {CMPI_RC_OK, NULL}; + CMPIInstance *inst; + const char *prop; + char* classname; + + classname = class_base_name(CLASSNAME(ref)); + + if (STREQC(classname, "VirtualSystemManagementService")) { + s = get_vsms(ref, &inst, _BROKER); + } + else if (STREQC(classname, "ResourcePoolConfigurationService")) { + s = rpcs_instance(ref, &inst, _BROKER); + } + else if (STREQC(classname, "VirtualSystemMigrationService")) { + s = get_migration_service(ref, &inst, _BROKER); + } + if (s.rc != CMPI_RC_OK) + return s; + + prop = cu_compare_ref(ref, inst); + if (prop != NULL) { + cu_statusf(_BROKER, &s, + CMPI_RC_ERR_NOT_FOUND, + "No such instance (%s)", prop); + } + + free(classname); + + return s; +} + static CMPIStatus service_to_host(const CMPIObjectPath *ref, struct std_assoc_info *info, struct inst_list *list) @@ -47,6 +98,10 @@ static CMPIStatus service_to_host(const if (!match_hypervisor_prefix(ref, info)) return s; + s = check_service_ref(ref); + if (s.rc != CMPI_RC_OK) + return s; + s = get_host_cs(_BROKER, ref, &instance); if (s.rc == CMPI_RC_OK) inst_list_add(list, instance); @@ -62,6 +117,10 @@ static CMPIStatus host_to_service(const CMPIInstance *inst; if (!match_hypervisor_prefix(ref, info)) + return s; + + s = check_host_ref(ref); + if (s.rc != CMPI_RC_OK) return s; s = rpcs_instance(ref, &inst, _BROKER);
Heidi Eckhart wrote:
# HG changeset patch # User Heidi Eckhart <heidieck@linux.vnet.ibm.com> # Date 1199797461 -3600 # Node ID 555126035e912e9202ebcc93483ee936be5ae998 # Parent 1bbea87ad37dcb95321f43aa5e17e6fa7ec1836d HS: returns results for wrong object path
The client given reference was not checked for existance in both directions (from HostSystem to Service and reverse) and the call returned wrong results. Now NOT_FOUND is returned in case of a wrong object path.
Examples: wbemcli ain -ac KVM_HostedService 'http://localhost/root/virt:KVM_ResourcePoolConfigurationService.CreationClassName="wrong",wrong="wrong"' wbemcli ain -ac KVM_HostedService 'http://localhost/root/virt:KVM_HostSystem.CreationClassName="KVM_HostSystem",Name="notthere"'
Signed-off-by: Heidi Eckhart <heidieck@linux.vnet.ibm.com>
diff -r 1bbea87ad37d -r 555126035e91 src/Virt_HostedService.c --- a/src/Virt_HostedService.c Mon Jan 07 11:42:40 2008 -0800 +++ b/src/Virt_HostedService.c Tue Jan 08 14:04:21 2008 +0100 @@ -37,6 +37,57 @@
+ +static CMPIStatus check_service_ref(const CMPIObjectPath *ref) +{ + CMPIStatus s = {CMPI_RC_OK, NULL}; + CMPIInstance *inst; + const char *prop; + char* classname; + + classname = class_base_name(CLASSNAME(ref)); + + if (STREQC(classname, "VirtualSystemManagementService")) { + s = get_vsms(ref, &inst, _BROKER); + } + else if (STREQC(classname, "ResourcePoolConfigurationService")) { + s = rpcs_instance(ref, &inst, _BROKER); + } + else if (STREQC(classname, "VirtualSystemMigrationService")) { + s = get_migration_service(ref, &inst, _BROKER); + } + if (s.rc != CMPI_RC_OK) + return s;
Sorry for being so nitpicky, but for the sake of consistency I think this is a place where we use a "goto out;" type of statement as opposed to having multiple returns.
+ + prop = cu_compare_ref(ref, inst); + if (prop != NULL) { + cu_statusf(_BROKER, &s, + CMPI_RC_ERR_NOT_FOUND, + "No such instance (%s)", prop); + } +
Putting out right here should work fine. Actually, this is more than a nitpick I think, because if the above return happens we don't hit this free() and leak classname.
+ free(classname); + + return s; +}
-- -Jay
Jay Gagnon wrote:
Heidi Eckhart wrote:
+ if (s.rc != CMPI_RC_OK) + return s;
Sorry for being so nitpicky, but for the sake of consistency I think this is a place where we use a "goto out;" type of statement as opposed to having multiple returns.
I wouldn't call it nitpicky. Its a very good finding :) ! Thanks.
+ + prop = cu_compare_ref(ref, inst); + if (prop != NULL) { + cu_statusf(_BROKER, &s, + CMPI_RC_ERR_NOT_FOUND, + "No such instance (%s)", prop); + } +
Putting out right here should work fine. Agree and done so in the updated patch. Actually, this is more than a nitpick I think, because if the above return happens we don't hit this free() and leak classname.
Mhh ... the above code does only set the status, but does not return right afterwards. So we wouldn't leak classname. Or did I miss something ?
+ free(classname); + + return s; +}
-- Regards Heidi Eckhart Software Engineer IBM Linux Technology Center - Open Hypervisor
Jay Gagnon wrote:
Heidi Eckhart wrote:
+ if (s.rc != CMPI_RC_OK) + return s;
Sorry for being so nitpicky, but for the sake of consistency I think this is a place where we use a "goto out;" type of statement as opposed to having multiple returns.
I wouldn't call it nitpicky. Its a very good finding :) ! Thanks.
+ + prop = cu_compare_ref(ref, inst); + if (prop != NULL) { + cu_statusf(_BROKER, &s, + CMPI_RC_ERR_NOT_FOUND, + "No such instance (%s)", prop); + } + Putting out right here should work fine. Agree and done so in the updated patch. Actually, this is more than a nitpick I think, because if the above return happens we don't hit this free() and leak classname.
Mhh ... the above code does only set the status, but does not return right afterwards. So we wouldn't leak classname. Or did I miss something ? Sorry, my "above" was a bit vague. I meant that if the "return s;" at
Heidi Eckhart wrote: the very top of this email happens, we leave the function before we get to "free(classname);" and so classname will be leaked. But no matter, the point will become moot when you switch the return to a goto; classname definitely won't leak that way. -- -Jay
HE> + if (STREQC(classname, "VirtualSystemManagementService")) { HE> + s = get_vsms(ref, &inst, _BROKER); HE> + } HE> + else if (STREQC(classname, "ResourcePoolConfigurationService")) { HE> + s = rpcs_instance(ref, &inst, _BROKER); HE> + } HE> + else if (STREQC(classname, "VirtualSystemMigrationService")) { HE> + s = get_migration_service(ref, &inst, _BROKER); HE> + } HE> + if (s.rc != CMPI_RC_OK) HE> + return s; This doesn't fit the style of the rest of the code. The "else if" statements should be on the same line as the closing brace, and there should be a blank line before the second and unrelated if. Otherwise, a good fix. Thanks! -- Dan Smith IBM Linux Technology Center Open Hypervisor Team email: danms@us.ibm.com
Dan Smith wrote:
HE> + if (STREQC(classname, "VirtualSystemManagementService")) { HE> + s = get_vsms(ref, &inst, _BROKER); HE> + } HE> + else if (STREQC(classname, "ResourcePoolConfigurationService")) { HE> + s = rpcs_instance(ref, &inst, _BROKER); HE> + } HE> + else if (STREQC(classname, "VirtualSystemMigrationService")) { HE> + s = get_migration_service(ref, &inst, _BROKER); HE> + } HE> + if (s.rc != CMPI_RC_OK) HE> + return s;
This doesn't fit the style of the rest of the code. The "else if" statements should be on the same line as the closing brace, and there should be a blank line before the second and unrelated if.
Fixed in the updated version. -- Regards Heidi Eckhart Software Engineer IBM Linux Technology Center - Open Hypervisor
participants (3)
-
Dan Smith -
Heidi Eckhart -
Jay Gagnon