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

Yi Li posted 1 patch 3 years ago
Test syntax-check failed
Patches applied successfully (tree, apply log)
git fetch https://github.com/patchew-project/libvirt tags/patchew/20210312095122.764594-1-yili@winhong.com
src/util/virconf.c | 47 +++++++++++++++-------------------------------
1 file changed, 15 insertions(+), 32 deletions(-)
[PATCH] virconf.c: Refactor cleanup and remove VIR_FREE
Posted by Yi Li 3 years ago
Switch to using the 'g_auto*' helpers.

Signed-off-by: Yi Li <yili@winhong.com>
---
 src/util/virconf.c | 47 +++++++++++++++-------------------------------
 1 file changed, 15 insertions(+), 32 deletions(-)

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;
 
     if (CUR != '#')
         return -1;
@@ -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;
-    }
+
     return 0;
 }
 
@@ -626,9 +625,9 @@ static int
 virConfParseStatement(virConfParserCtxtPtr ctxt)
 {
     const char *base;
-    char *name;
+    g_autofree char *name;
     virConfValuePtr value;
-    char *comm = NULL;
+    g_autofree char *comm = NULL;
 
     SKIP_BLANKS_AND_EOL;
     if (CUR == '#')
@@ -639,16 +638,13 @@ virConfParseStatement(virConfParserCtxtPtr ctxt)
     SKIP_BLANKS;
     if (CUR != '=') {
         virConfError(ctxt, VIR_ERR_CONF_SYNTAX, _("expecting an assignment"));
-        VIR_FREE(name);
         return -1;
     }
     NEXT;
     SKIP_BLANKS;
     value = virConfParseValue(ctxt);
-    if (value == NULL) {
-        VIR_FREE(name);
+    if (value == NULL)
         return -1;
-    }
     SKIP_BLANKS;
     if (CUR == '#') {
         NEXT;
@@ -657,9 +653,7 @@ virConfParseStatement(virConfParserCtxtPtr ctxt)
         comm = g_strndup(base, ctxt->cur - base);
     }
     if (virConfAddEntry(ctxt->conf, name, value, comm) == NULL) {
-        VIR_FREE(name);
         virConfFreeValue(value);
-        VIR_FREE(comm);
         return -1;
     }
     return 0;
@@ -724,7 +718,7 @@ virConfParse(const char *filename, const char *content, int len,
 virConfPtr
 virConfReadFile(const char *filename, unsigned int flags)
 {
-    char *content;
+    g_autofree char *content;
     int len;
     virConfPtr conf;
 
@@ -740,7 +734,6 @@ virConfReadFile(const char *filename, unsigned int flags)
 
     conf = virConfParse(filename, content, len, flags);
 
-    VIR_FREE(content);
 
     return conf;
 }
@@ -1412,7 +1405,7 @@ virConfWriteFile(const char *filename, virConfPtr conf)
     virConfEntryPtr cur;
     int ret;
     int fd;
-    char *content;
+    g_autofree char *content;
     unsigned int use;
 
     if (conf == NULL)
@@ -1433,7 +1426,6 @@ virConfWriteFile(const char *filename, virConfPtr conf)
     use = virBufferUse(&buf);
     content = virBufferContentAndReset(&buf);
     ret = safewrite(fd, content, use);
-    VIR_FREE(content);
     VIR_FORCE_CLOSE(fd);
     if (ret != (int)use) {
         virConfError(NULL, VIR_ERR_WRITE_FAILED, _("failed to save content"));
@@ -1461,7 +1453,7 @@ virConfWriteMem(char *memory, int *len, virConfPtr conf)
 {
     g_auto(virBuffer) buf = VIR_BUFFER_INITIALIZER;
     virConfEntryPtr cur;
-    char *content;
+    g_autofree char *content;
     unsigned int use;
 
     if ((memory == NULL) || (len == NULL) || (*len <= 0) || (conf == NULL))
@@ -1478,11 +1470,9 @@ virConfWriteMem(char *memory, int *len, virConfPtr conf)
 
     if ((int)use >= *len) {
         *len = (int)use;
-        VIR_FREE(content);
         return -1;
     }
     memcpy(memory, content, use);
-    VIR_FREE(content);
     *len = use;
     return use;
 }
@@ -1505,26 +1495,19 @@ virConfLoadConfigPath(const char *name)
 int
 virConfLoadConfig(virConfPtr *conf, const char *name)
 {
-    char *path = NULL;
-    int ret = -1;
+    g_autofree char *path = NULL;
 
     *conf = NULL;
 
     if (!(path = virConfLoadConfigPath(name)))
-        goto cleanup;
+        return -1;
 
-    if (!virFileExists(path)) {
-        ret = 0;
-        goto cleanup;
-    }
+    if (!virFileExists(path))
+        return 0;
 
     VIR_DEBUG("Loading config file '%s'", path);
     if (!(*conf = virConfReadFile(path, 0)))
-        goto cleanup;
-
-    ret = 0;
+        return -1;
 
- cleanup:
-    VIR_FREE(path);
-    return ret;
+    return 0;
 }
-- 
2.25.3




Re: [PATCH] virconf.c: Refactor cleanup and remove VIR_FREE
Posted by Michal Privoznik 3 years ago
On 3/12/21 10:51 AM, Yi Li wrote:
> Switch to using the 'g_auto*' helpers.
> 
> Signed-off-by: Yi Li <yili@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