[libvirt] [PATCH 0/3] Couple of virBuffer fixes
Almost trivial. Michal Prívozník (3): virbuffer: Don't leak memory in virBufferAddBuffer virbuffer: Use signed integer for storing error virBuffer: Try harder to free buffer src/util/virbuffer.c | 7 +++---- src/util/virbuffer.h | 2 +- tests/virbuftest.c | 32 ++++++++++++++++++++++++++++++++ 3 files changed, 36 insertions(+), 5 deletions(-) -- 2.21.0
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
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.
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
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
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
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
The @error member can contain a positive value (errno) or a negative value (-1) to denote an usage error. It doesn't make much sense to store it as unsigned then. Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- src/util/virbuffer.h | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/util/virbuffer.h b/src/util/virbuffer.h index fbfed5cbf3..dd9d4ab3d4 100644 --- a/src/util/virbuffer.h +++ b/src/util/virbuffer.h @@ -40,7 +40,7 @@ typedef virBuffer *virBufferPtr; struct _virBuffer { size_t size; size_t use; - unsigned int error; /* errno value, or -1 for usage error */ + int error; /* errno value, or -1 for usage error */ int indent; char *content; }; -- 2.21.0
On Thu, Apr 18, 2019 at 02:11:24PM +0200, Michal Privoznik wrote:
The @error member can contain a positive value (errno) or a negative value (-1) to denote an usage error. It doesn't make
much sense to store it as unsigned then.
Signed-off-by: Michal Privoznik <mprivozn@redhat.com> ---
s/an usage/a usage --> even though there are 2 vowels in a row, pronounciation is important, since you pronounce it as [ˈjuːzɪʤ], you need 'a' rather than 'an'. An opposite example would be 'a hour' vs 'an hour' where the latter is correct because of the vowel sound in pronouncing 'hour'. trivial Reviewed-by: Erik Skultety <eskultet@redhat.com>
On 5/3/19 10:30 AM, Erik Skultety wrote:
On Thu, Apr 18, 2019 at 02:11:24PM +0200, Michal Privoznik wrote:
The @error member can contain a positive value (errno) or a negative value (-1) to denote an usage error. It doesn't make
s/an usage/a usage
--> even though there are 2 vowels in a row, pronounciation is important, since you pronounce it as [ˈjuːzɪʤ], you need 'a' rather than 'an'. An opposite example would be 'a hour' vs 'an hour' where the latter is correct because of the vowel sound in pronouncing 'hour'.
https://en.wikipedia.org/wiki/Ghoti
much sense to store it as unsigned then.
Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- trivial
Reviewed-by: Erik Skultety <eskultet@redhat.com>
Thanks, Michal
Currently, the way virBufferFreeAndReset() works is it relies on virBufferContentAndReset() to fetch the buffer content which is then freed. This works as long as there is no bug in virBuffer* implementation (not true apparently). Explicitly call free() over buffer content. Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- src/util/virbuffer.c | 5 ++--- 1 file changed, 2 insertions(+), 3 deletions(-) diff --git a/src/util/virbuffer.c b/src/util/virbuffer.c index b2ae7963a1..ac03b15a61 100644 --- a/src/util/virbuffer.c +++ b/src/util/virbuffer.c @@ -281,9 +281,8 @@ virBufferContentAndReset(virBufferPtr buf) */ void virBufferFreeAndReset(virBufferPtr buf) { - char *str = virBufferContentAndReset(buf); - - VIR_FREE(str); + if (buf) + virBufferSetError(buf, 0); } /** -- 2.21.0
On Thu, Apr 18, 2019 at 02:11:25PM +0200, Michal Privoznik wrote:
Currently, the way virBufferFreeAndReset() works is it relies on virBufferContentAndReset() to fetch the buffer content which is then freed. This works as long as there is no bug in virBuffer* implementation (not true apparently). Explicitly call free() over buffer content.
Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- src/util/virbuffer.c | 5 ++--- 1 file changed, 2 insertions(+), 3 deletions(-)
diff --git a/src/util/virbuffer.c b/src/util/virbuffer.c index b2ae7963a1..ac03b15a61 100644 --- a/src/util/virbuffer.c +++ b/src/util/virbuffer.c @@ -281,9 +281,8 @@ virBufferContentAndReset(virBufferPtr buf) */ void virBufferFreeAndReset(virBufferPtr buf) { - char *str = virBufferContentAndReset(buf); - - VIR_FREE(str); + if (buf) + virBufferSetError(buf, 0);
You saved 1 call to memset. I also don't see how we can break virBufferContentAndReset in a way that we couldn't mess up virBufferSetError in the same way. Additionally, seeing a call to virBufferSetError in a free helper looks suspicious (even if it has a '0' as argument). If anything, I'd rename virBuggerFreeAndReset to virBufferClean. Erik
On 5/3/19 11:01 AM, Erik Skultety wrote:
On Thu, Apr 18, 2019 at 02:11:25PM +0200, Michal Privoznik wrote:
Currently, the way virBufferFreeAndReset() works is it relies on virBufferContentAndReset() to fetch the buffer content which is then freed. This works as long as there is no bug in virBuffer* implementation (not true apparently). Explicitly call free() over buffer content.
Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- src/util/virbuffer.c | 5 ++--- 1 file changed, 2 insertions(+), 3 deletions(-)
diff --git a/src/util/virbuffer.c b/src/util/virbuffer.c index b2ae7963a1..ac03b15a61 100644 --- a/src/util/virbuffer.c +++ b/src/util/virbuffer.c @@ -281,9 +281,8 @@ virBufferContentAndReset(virBufferPtr buf) */ void virBufferFreeAndReset(virBufferPtr buf) { - char *str = virBufferContentAndReset(buf); - - VIR_FREE(str); + if (buf) + virBufferSetError(buf, 0);
You saved 1 call to memset. I also don't see how we can break virBufferContentAndReset in a way that we couldn't mess up virBufferSetError in the same way. Additionally, seeing a call to virBufferSetError in a free helper looks suspicious (even if it has a '0' as argument). If anything, I'd rename virBuggerFreeAndReset to virBufferClean.
Thing is, if 1/3 wasn't merged, then the test I'm introducing there would leak memory. But not with this patch merged. So from long term perspective, if there is ever some similar bug we won't leak any memory. Yeah, naming is hard. Michal
On Fri, May 03, 2019 at 11:18:09AM +0200, Michal Privoznik wrote:
On 5/3/19 11:01 AM, Erik Skultety wrote:
On Thu, Apr 18, 2019 at 02:11:25PM +0200, Michal Privoznik wrote:
Currently, the way virBufferFreeAndReset() works is it relies on virBufferContentAndReset() to fetch the buffer content which is then freed. This works as long as there is no bug in virBuffer* implementation (not true apparently). Explicitly call free() over buffer content.
Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- src/util/virbuffer.c | 5 ++--- 1 file changed, 2 insertions(+), 3 deletions(-)
diff --git a/src/util/virbuffer.c b/src/util/virbuffer.c index b2ae7963a1..ac03b15a61 100644 --- a/src/util/virbuffer.c +++ b/src/util/virbuffer.c @@ -281,9 +281,8 @@ virBufferContentAndReset(virBufferPtr buf) */ void virBufferFreeAndReset(virBufferPtr buf) { - char *str = virBufferContentAndReset(buf); - - VIR_FREE(str); + if (buf) + virBufferSetError(buf, 0);
You saved 1 call to memset. I also don't see how we can break virBufferContentAndReset in a way that we couldn't mess up virBufferSetError in the same way. Additionally, seeing a call to virBufferSetError in a free helper looks suspicious (even if it has a '0' as argument). If anything, I'd rename virBuggerFreeAndReset to virBufferClean.
Thing is, if 1/3 wasn't merged, then the test I'm introducing there would leak memory. But not with this patch merged. So from long term perspective, if there is ever some similar bug we won't leak any memory.
Yeah, naming is hard.
Reviewed-by: Erik Skultety <eskultet@redhat.com>
participants (3)
-
Andrea Bolognani -
Erik Skultety -
Michal Privoznik