[libvirt] [PATCH 1/3] virbuffer: Don't leak memory in virBufferAddBuffer

[Michal Privoznik]

On 5/3/19 11:18 AM, Andrea Bolognani wrote:
> On Thu, 2019-04-18 at 14:11 +0200, Michal Privoznik wrote:
>> If an error occurs in a virBuffer* API the idea is to free the
>> content immediately and set @error member used in error reporting
>> later. Well, this is not what how virBufferAddBuffer works.
>>
>> Signed-off-by: Michal Privoznik <mprivozn at redhat.com>
>> ---
>>   src/util/virbuffer.c |  2 +-
>>   tests/virbuftest.c   | 32 ++++++++++++++++++++++++++++++++
>>   2 files changed, 33 insertions(+), 1 deletion(-)
> 
> I'm a bit confused, so I'll spell everything out and you can tell me
> what I'm getting wrong :)
> 
>> @@ -197,7 +197,7 @@ virBufferAddBuffer(virBufferPtr buf, virBufferPtr toadd)
>>   
>>       if (buf->error || toadd->error) {
>>           if (!buf->error)
>> -            buf->error = toadd->error;
>> +            virBufferSetError(buf, toadd->error);
>>           goto done;
>>       }
> 
> This change makes sense to me: instead of simply setting the error
> flag in the destination buffer, we use virBufferSetError() so that
> we will set the error flag *and* also free all memory associated
> with the buffer. This is consistent with the commit message and the
> comments in the newly-added test case, where you claim you're
> addressing a memory leak. So far, so good.
> 
> [...]
>> +static int
>> +testBufAddBuffer2(const void *opaque ATTRIBUTE_UNUSED)
>> +{
>> +    virBuffer buf1 = VIR_BUFFER_INITIALIZER;
>> +    virBuffer buf2 = VIR_BUFFER_INITIALIZER;
>> +    int ret = -1;
>> +
>> +    /* Intent of this test is to demonstrate a memleak that happen with
>> +     * virBufferAddBuffer */
>> +
>> +    virBufferAddLit(&buf1, "Hello world!\n");
>> +    virBufferAddLit(&buf2, "Hello world!\n");
> 
> Here you're adding some data to the buffers. They're both in a sane
> state for the time being.
> 
>> +    /* Intentional usage error */
>> +    virBufferAdjustIndent(&buf2, -2);
> 
> Here you reduce by 2 the indentation of buf2; however, since the
> indentation for the buffer was 0 before the call, this is not a
> valid use of the API: the memory allocated to buf2 is released, and
> the error flag for it is set.

Yes, as the comment says, this is intentional. The idea is to make the 
very next call fail.

> 
>> +    virBufferAddBuffer(&buf1, &buf2);
> 
> Now you try to add the (errored) buf2 to buf1, which should result
> in buf1 being unallocated and put into an error state as well. So
> at this point the memory allocated to both buffers should have been
> released, and the error flag should have been set for both.
> 
>> +    if (virBufferCurrentContent(&buf1) ||
>> +        !virBufferCurrentContent(&buf2)) {
>> +        VIR_TEST_DEBUG("Unexpected buffer content");
>> +        goto cleanup;
>> +    }
> 
> And here you check both buffers. This is the part I don't get: since
> virBufferCurrentContent() returns NULL for errored buffers, shouldn't
> we get NULL in both cases here? And should we not get that result
> both with and *without* your patch? We were already setting the error
> flag for buf1 after all...
> 
> I tried reverting the changes to virBufferAddBuffer() made in this
> commit, and 'make check' still passes for me, which matches my
> observation above but certainly not my original expectations.

Firstly, it's not about passing the test but about leaking memory. Try 
running under valgrind.
Secondly, virBufferAddBuffer() resets the other buffer, so even if it 
had an error it no longer has it after virBufferAddBuffer() is called. 
And no error means virBufferCurrentContent() returns an empty string 
(because the buffer has no content).

Hopefully this clears your concerns.

> 
>> +    ret = 0;
>> + cleanup:
>> +    virBufferFreeAndReset(&buf1);
>> +    virBufferFreeAndReset(&buf2);
>> +    return ret;
> 
> Stray observation: you can use VIR_AUTOCLEAR() when declaring the
> buffers and drop the cleanup path entirely.
> 

Michal