[Libvirt-cim] [PATCH 2/6] libxkutil:pool_parsing: Coverity cleanups
Boris Fiuczynski
fiuczy at linux.vnet.ibm.com
Wed Feb 5 16:04:34 UTC 2014
On 02/05/2014 03:08 PM, John Ferlan wrote:
>
>
> On 02/04/2014 07:31 AM, Boris Fiuczynski wrote:
>> On 01/22/2014 08:30 PM, John Ferlan wrote:
>>> A new version of Coverity found a number of issues:
>>>
>>> get_pool_from_xml(): FORWARD_NULL
>>> parse_disk_pool(): RESOURCE_LEAK
>>> - If parse_disk_pool() returned name == NULL, then the strdup() of
>>> name into pool->id would dereference the NULL pointer leading to
>>> a segfault.
>>>
>>> Furthermore parse_disk_pool() had a few issues. First the 'type_str'
>>> could be NULL, so that needs to be checked. Second, 'type_str' was
>>> never free()'d (the get_attr_value returns a strdup()'d value).
>>>
>>> Realizing all that resulted in a few extra changes to not strdup()
>>> a value that we strdup()'d
>>>
>>> Eventually get_pool_from_xml() will return to get_disk_pool() which
>>> returns to diskpool_set_capacity() or _new_volume_template() which
>>> both error out when return value != 0 (although, I did change the
>>> latter to be more explicit and match the former).
>>>
>>> Finally, even though the parsing of the element will only ever have
>>> one "name" value - Coverity doesn't know that, so as a benign fix be
>>> sure to not overwrite 'name' if "name" isn't already set.
>>>
>>> Signed-off-by: John Ferlan <jferlan at redhat.com>
>>> ---
>>> libxkutil/pool_parsing.c | 39 ++++++++++++++++++-----------------
>>> src/Virt_SettingsDefineCapabilities.c | 2 +-
>>> 2 files changed, 21 insertions(+), 20 deletions(-)
>>>
>>> diff --git a/libxkutil/pool_parsing.c b/libxkutil/pool_parsing.c
>>> index 922ff32..80bd4ca 100644
>>> --- a/libxkutil/pool_parsing.c
>>> +++ b/libxkutil/pool_parsing.c
>>> @@ -194,15 +194,17 @@ char *get_disk_pool_type(uint16_t type)
>>>
>>> }
>>>
>>> -static const char *parse_disk_pool(xmlNodeSet *nsv, struct disk_pool *pool)
>>> +static char *parse_disk_pool(xmlNodeSet *nsv, struct disk_pool *pool)
>>> {
>>> xmlNode **nodes = nsv->nodeTab;
>>> xmlNode *child;
>>> - const char *type_str = NULL;
>>> - const char *name = NULL;
>>> + char *type_str = NULL;
>>> + char *name = NULL;
>>> int type = 0;
>>>
>>> type_str = get_attr_value(nodes[0], "type");
>>> + if (type_str == NULL)
>>> + return NULL;
>>>
>>> if (STREQC(type_str, "dir"))
>>> type = DISK_POOL_DIR;
>>> @@ -220,12 +222,15 @@ static const char *parse_disk_pool(xmlNodeSet *nsv, struct disk_pool *pool)
>>> type = DISK_POOL_SCSI;
>>> else
>>> type = DISK_POOL_UNKNOWN;
>>> + free(type_str);
>>>
>>> pool->pool_type = type;
>>> -
>>> +
>>> for (child = nodes[0]->children; child != NULL; child = child->next) {
>>> - if (XSTREQ(child->name, "name")) {
>>> + if (XSTREQ(child->name, "name") && name == NULL) {
>>> name = get_node_content(child);
>>> + if (name == NULL)
>>> + return NULL;
>>> } else if (XSTREQ(child->name, "target"))
>>> parse_disk_target(child, pool);
>>> else if (XSTREQ(child->name, "source"))
>>> @@ -238,14 +243,18 @@ static const char *parse_disk_pool(xmlNodeSet *nsv, struct disk_pool *pool)
>>> int get_pool_from_xml(const char *xml, struct virt_pool *pool, int type)
>>> {
>>> int len;
>>> - int ret = 0;
>>> + int ret = 1;
>> Isn't this a bug fix... beside a cleanup? Before the return value was
>> always 0! It also changes the method get_disk_pool in Virt_DevicePool.c
>> in its return value behavior in the same way.
>
> True - a bug fix insomuch as either caller is concerned, but yet still
> found by Coverity as a RESOURCE_LEAK initially and then me as I was
> reading the code... I think the primary way to hit the bug would have
> been through a *alloc()/strdup() failure though.
>
>> The usage of get_disk_pool in the method diskpool_set_capacity checks
>> for the return value and being !=0 it is considered an error
>> (Virt_DevicePool.c line 324). Doesn't that need to get changed in this
>> patch?
>
> I don't think so. Prior to my change/cleanup if anything failed in
> get_pool_from_xml(), then diskpool_set_capacity() could wrongly or even
> partially set the properties for "Path", "Host", "SourceDirectory", and
> "OtherResourceType". Although perhaps only the type would be wrong,
> since all others actually check for non NULL fields before strdup() and
> CMSetProperty() calls. Thus perhaps "path" could be set, but host and
> source directory not due to memory constraints, or some combination of
> the 3. Probably not a good thing to be happening. Of course there are
> those that contend that if you're that memory constrained, the code
> isn't going much further anyway...
>
> In any case, all my change does is acknowledge and return the failure
> which the caller already handled properly.
>
> FWIW: For context
>
> Line 324 in diskpool_set_capacity():
>
> if (get_disk_pool(pool, &pool_vals) != 0) {
> CU_DEBUG("Error getting pool path for: %s", _pool->tag);
> } else {
> if (pool_vals->pool_info.disk.path != NULL) {
> pool_str = strdup(pool_vals->pool_info.disk.path);
> CMSetProperty(inst, "Path",
> (CMPIValue *)pool_str, CMPI_chars);
> }
> ...
>
Sorry, but you are right. I missed one line in get_pool_from_xml that
screwed up my logic. Works fine.
I also did run cimtest on my system and it remained the same.
>
>> Even so you mention above in your description the two chained
>> exploitation code pieces it seems that you fixed just one ... or did I
>> miss something?
>>
>
> The reason for the change in _new_volume_template() was to match the
> return comparison from diskpool_set_capacity()... It's unnecessary
> technically. One used to fail on "ret == 1" and the other when "ret !=
> 0". Now both fail in the ret != 0 case. It's the "Type A personality"
> in me that does that.
>
> John
>
>
>
>>> xmlDoc *xmldoc;
>>> xmlXPathContext *xpathctx;
>>> xmlXPathObject *xpathobj;
>>> const xmlChar *xpathstr = (xmlChar *)"/pool";
>>> - const char *name;
>>>
>>> CU_DEBUG("Pool XML : %s", xml);
>>> +
>>> + /* FIXME: Add support for parsing network pools */
>>> + if (type == CIM_RES_TYPE_NET)
>>> + return 0;
>>> +
>>> len = strlen(xml) + 1;
>>>
>>> if ((xmldoc = xmlParseMemory(xml, len)) == NULL)
>>> @@ -257,22 +266,14 @@ int get_pool_from_xml(const char *xml, struct virt_pool *pool, int type)
>>> if ((xpathobj = xmlXPathEvalExpression(xpathstr, xpathctx)) == NULL)
>>> goto err3;
>>>
>>> - /* FIXME: Add support for parsing network pools */
>>> - if (type == CIM_RES_TYPE_NET) {
>>> - ret = 0;
>>> - goto err1;
>>> - }
>>> -
>>> memset(pool, 0, sizeof(*pool));
>>>
>>> - pool->type = CIM_RES_TYPE_DISK;
>>> - name = parse_disk_pool(xpathobj->nodesetval,
>>> - &(pool)->pool_info.disk);
>>> - if (name == NULL)
>>> + pool->type = CIM_RES_TYPE_DISK;
>>> + pool->id = parse_disk_pool(xpathobj->nodesetval,
>>> + &(pool)->pool_info.disk);
>>> + if (pool->id != NULL)
>>> ret = 0;
>>>
>>> - pool->id = strdup(name);
>>> -
>>> xmlXPathFreeObject(xpathobj);
>>> err3:
>>> xmlXPathFreeContext(xpathctx);
>>> diff --git a/src/Virt_SettingsDefineCapabilities.c b/src/Virt_SettingsDefineCapabilities.c
>>> index fe16e3f..756e46b 100644
>>> --- a/src/Virt_SettingsDefineCapabilities.c
>>> +++ b/src/Virt_SettingsDefineCapabilities.c
>>> @@ -1376,7 +1376,7 @@ static CMPIStatus _new_volume_template(const CMPIObjectPath *ref,
>>> }
>>>
>>> ret = get_disk_pool(poolptr, &pool);
>>> - if (ret == 1) {
>>> + if (ret != 0) {
>>> virt_set_status(_BROKER, &s,
>>> CMPI_RC_ERR_FAILED,
>>> virStoragePoolGetConnect(poolptr),
>>>
>> src/Virt_DevicePool.c line 324 =! change seems to be missing.
>>
>>
>
> _______________________________________________
> Libvirt-cim mailing list
> Libvirt-cim at redhat.com
> https://www.redhat.com/mailman/listinfo/libvirt-cim
>
--
Mit freundlichen Grüßen/Kind regards
Boris Fiuczynski
IBM Deutschland Research & Development GmbH
Vorsitzender des Aufsichtsrats: Martina Köderitz
Geschäftsführung: Dirk Wittkopp
Sitz der Gesellschaft: Böblingen
Registergericht: Amtsgericht Stuttgart, HRB 243294
More information about the Libvirt-cim
mailing list