Re: [PATCH] virconf.c: Refactor cleanup and remove VIR_FREE
On 3/12/21 10:51 AM, Yi Li wrote:
Switch to using the 'g_auto*' helpers.
Signed-off-by: Yi Li <yili(a)winhong.com>
---
src/util/virconf.c | 47 +++++++++++++++-------------------------------
1 file changed, 15 insertions(+), 32 deletions(-)
Almost :-)
diff --git a/src/util/virconf.c b/src/util/virconf.c
index 16107bce96..17fbea2397 100644
--- a/src/util/virconf.c
+++ b/src/util/virconf.c
@@ -573,7 +573,7 @@ static int
virConfParseComment(virConfParserCtxtPtr ctxt)
{
const char *base;
- char *comm;
+ g_autofree char *comm;
This leaves @comm uninitialized, therefore comm is assigned "random"
value (whatever was on the stack earlier).
if (CUR != '#')
return -1;
And thus if this branch is taken, the autofree kicks in (the variable is
going out of scope) and frees random pointer. You can imagine how
dangerous that is.
@@ -581,10 +581,9 @@ virConfParseComment(virConfParserCtxtPtr ctxt)
base = ctxt->cur;
while ((ctxt->cur < ctxt->end) && (!IS_EOL(CUR))) NEXT;
comm = g_strndup(base, ctxt->cur - base);
- if (virConfAddEntry(ctxt->conf, NULL, NULL, comm) == NULL) {
- VIR_FREE(comm);
+ if (virConfAddEntry(ctxt->conf, NULL, NULL, comm) == NULL)
return -1;
- }
+
And here, @comm was consumed by virConfAddEntry() - it was added at the
end of ctxt->conf list. Therefore, we must refrain from freeing it here.
Otherwise we leave a pointer behind (at the end of the list) that points
to a memory that was freed. One way to avoid this, is to set comm
explicitly to NULL (with a comment that virConfAddEntry() consumed it),
because then when g_autofree comes and calls free() it does so over a
NULL pointer which is defined to be a NO-OP.
However, I think the cleaner solution is to modify virConfAddEntry() so
that it doesn't take just the first degree pointer, but the second
degree. Because then it's clear to see that ownership of the memory is
transferred - just by looking at the call:
virConfAddEntry(ctxt->conf, NULL, NULL, &comm);
Inside of virConfAddEntry() g_steal_pointer() can be used for assignment
and clearing *comm, like this:
ret->comment = g_steal_pointer(comm);
Of course, this applies transitively to other arguments (unfortunately,
here only NULL is passed, but you get the idea).
BTW: this was caught also by our testsuite (ninja -C _build test) -
virconftest is crashing with this patch.
Michal