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