[PATCH] virconf.c: Refactor cleanup and remove VIR_FREE

[Michal Privoznik]

On 3/12/21 10:51 AM, Yi Li wrote:
> Switch to using the 'g_auto*' helpers.
> 
> Signed-off-by: Yi Li <yili at 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