Re: [libvirt] [PATCH 1/3] virbuffer: Don't leak memory in virBufferAddBuffer
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(a)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