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@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;
     }
 
diff --git a/tests/virbuftest.c b/tests/virbuftest.c
index 778754d7c1..bdd0c16462 100644
--- a/tests/virbuftest.c
+++ b/tests/virbuftest.c
@@ -303,6 +303,37 @@ static int testBufAddBuffer(const void *data ATTRIBUTE_UNUSED)
     return ret;
 }
 
+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");
+
+    /* Intentional usage error */
+    virBufferAdjustIndent(&buf2, -2);
+
+    virBufferAddBuffer(&buf1, &buf2);
+
+    if (virBufferCurrentContent(&buf1) ||
+        !virBufferCurrentContent(&buf2)) {
+        VIR_TEST_DEBUG("Unexpected buffer content");
+        goto cleanup;
+    }
+
+    ret = 0;
+ cleanup:
+    virBufferFreeAndReset(&buf1);
+    virBufferFreeAndReset(&buf2);
+    return ret;
+}
+
 struct testBufAddStrData {
     const char *data;
     const char *expect;
@@ -460,6 +491,7 @@ mymain(void)
     DO_TEST("Auto-indentation", testBufAutoIndent, 0);
     DO_TEST("Trim", testBufTrim, 0);
     DO_TEST("AddBuffer", testBufAddBuffer, 0);
+    DO_TEST("AddBuffer2", testBufAddBuffer2, 0);
     DO_TEST("set indent", testBufSetIndent, 0);
     DO_TEST("autoclean", testBufferAutoclean, 0);
 
-- 
2.21.0

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

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@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.

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).

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

But I'd like to get an opinion on the above, because I'm curious.

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

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@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@redhat.com>

Thanks,
Michal

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

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@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.

> +    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.

> +    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.

-- 
Andrea Bolognani / Red Hat / Virtualization

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

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@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

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

On Fri, 2019-05-03 at 11:57 +0200, Michal Privoznik wrote:
> On 5/3/19 11:18 AM, Andrea Bolognani wrote:
> > On Thu, 2019-04-18 at 14:11 +0200, Michal Privoznik wrote:
> > > +    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.

Okay, this is the first bit that I was missing, though I suspected
something along those lines. FWIW it would have been more obvious,
at least to me, what was going on if you had introduced the test
first, in a separate commit, and then fixed the function, but the
way you're doing it is just fine. I blame Friday, and not having
had much coffee in the morning :)

> 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).

This is the second, and most important, bit I was missing: the fact
that resetting the buffer also resets the error flag. It was right
in front of my eyes, but I was not seeing it... See above again for
remarks about weekdays and beverages.

> Hopefully this clears your concerns.

It sure does! Thanks for taking the time to explain in detail.

  Reviewed-by: Andrea Bolognani <abologna@redhat.com>

whether or not you switch to VIR_AUTOCLEAN() - but please do :)

-- 
Andrea Bolognani / Red Hat / Virtualization

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list