[libvirt] [PATCH] conf: Fix memory leaks in virStoragePoolDefParseSource

Peter Krempa pkrempa at redhat.com
Wed May 9 09:50:36 UTC 2012


On 05/09/2012 10:45 AM, Alex Jia wrote:
> On 05/09/2012 04:06 PM, Peter Krempa wrote:
>> The problem with the added line is not visible with this context, so
>> I'm adding
>> some more:
>>
>> source->nhost = virXPathNodeSet("./host", ctxt,&nodeset);
>>
>> if (source->nhost) {
>> if (VIR_ALLOC_N(source->hosts, source->nhost)< 0) {
>> virReportOOMError();
>> goto cleanup;
>> }
>>
>> for (i = 0 ; i< source->nhost ; i++) {
>> name = virXMLPropString(nodeset[i], "name");
>> if (name == NULL) {
>> virStorageReportError(VIR_ERR_XML_ERROR,
>> "%s", _("missing storage pool host name"));
>> goto cleanup;
>> }
>> source->hosts[i].name = name;
>>
>> port = virXMLPropString(nodeset[i], "port");
>> if (port) {
>> if (virStrToLong_i(port, NULL, 10,&source->hosts[i].port)< 0) {
>> virStorageReportError(VIR_ERR_XML_ERROR,
>> _("Invalid port number: %s"),
>> port);
>> goto cleanup;
>> }
>> }
>> + VIR_FREE(nodeset);
>> }
>> }
>>
>> You added the VIR_FREE inside the for loop, so it gets freed before
>> the next iteration
>> and might cause a segfault in the second iteration.
> Although I haven't meet a segfault error, I think you're right, thanks.

You were lucky and the loop iterated just once. If the xml contained 
more "host" elements you'd hit the SIGSEGV. The correct point to free 
the nodeset is _after_ the for loop.

Peter




More information about the libvir-list mailing list