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

[Michal Privoznik]

On 5/3/19 10:40 AM, Erik Skultety wrote:
> On Thu, Apr 18, 2019 at 02:11:23PM +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(-)
>>
>> diff --git a/src/util/virbuffer.c b/src/util/virbuffer.c
>> index 54703a28d8..b2ae7963a1 100644
>> --- a/src/util/virbuffer.c
>> +++ b/src/util/virbuffer.c
>> @@ -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 indeed fixes the problem. But looking at the test below it got me
> wondering, why we actually define the functions as void and free the original
> buffer on error with setting a numerical error. We usually leave the cleanup to
> the caller if something goes wrong. Failing to add to a buffer is IMHO quite a
> serious problem, because it can be either allocation problem (in which case
> there are much  bigger problems) or an internal problem which also cannot be
> ignored because we store data in there so it opens a door to not only
> corruption but also inconsistency.
> 

The idea of virBuffer is to just write all the calls and only then check 
for an error. For instance like this:

virBuffer buf = VIR_BUFFER_INITIALIZER;
virBuffer buf2 = VIR_BUFFER_INITIALIZER;

virBufferAsprintf(&buf, "Hello world!");
virBufferAddLit(&buf, "\n");

virBufferAsprintf(&buf2, "Child reported:\n");
virBufferAddBuffer(&buf2, &buf);

virBufferChecError(&buf2);

We just call virBuffer*() and only at the end, just before we'd get its 
content check for an error. Note, that any of those calls can fail (e.g. 
due to OOM) but they're still declared as void.

Failing to add a buffer is not different to adding just any other string 
really.


> What I'd expect is that if the operation fails, we return -1, caller decides
> for themself whether they want to ignore or act upon it; the data is inherently
> invalidated, but they still should be able to cleanup the mess. Optionally, we
> could go one safety step further and not touch caller provided data to be
> modified until the last moment where we just swap pointers and thus the
> original isn't corrupted (we also do this at many places, but I understand
> "nice-to-have's" bring a certain burden and potential issues on their own).

Not really. What difference does it make if the error is checked for 
later? If there's an error, it is stored inside the virBuffer struct and 
all subsequent virBuffer*() calls turn to NOPs. Except 
virBufferCheckError() which fetches the recorded error.

> 
> To the patch:
> Reviewed-by: Erik Skultety <eskultet at redhat.com>

Thanks,
Michal