Re: [libvirt] [PATCH 1/3] virbuffer: Don't leak memory in virBufferAddBuffer
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(a)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(a)redhat.com>
But I'd like to get an opinion on the above, because I'm curious.