1. Patchew
  2. QEMU
  3. View series

[PATCH] qga: fix possible memory leak

luzhipeng posted 1 patch 1 year, 7 months ago
Patches applied successfully (tree, apply log)
git fetch https://github.com/patchew-project/qemu tags/patchew/20220921142036.1758-1-luzhipeng@cestc.cn
Maintainers: Michael Roth <michael.roth@amd.com>, Konstantin Kostiuk <kkostiuk@redhat.com>
qga/main.c | 19 ++++++++++++++-----
1 file changed, 14 insertions(+), 5 deletions(-)
[PATCH] qga: fix possible memory leak
Posted by luzhipeng 1 year, 7 months ago 
From: lu zhipeng <luzhipeng@cestc.cn>

Signed-off-by: lu zhipeng <luzhipeng@cestc.cn>
---
 qga/main.c | 19 ++++++++++++++-----
 1 file changed, 14 insertions(+), 5 deletions(-)

diff --git a/qga/main.c b/qga/main.c
index 5f1efa2333..73ea1aae65 100644
--- a/qga/main.c
+++ b/qga/main.c
@@ -1287,7 +1287,7 @@ static GAState *initialize_agent(GAConfig *config, int socket_activation)
     if (g_mkdir_with_parents(config->state_dir, S_IRWXU) == -1) {
         g_critical("unable to create (an ancestor of) the state directory"
                    " '%s': %s", config->state_dir, strerror(errno));
-        return NULL;
+        goto failed;
     }
 #endif
 
@@ -1312,7 +1312,7 @@ static GAState *initialize_agent(GAConfig *config, int socket_activation)
             if (!log_file) {
                 g_critical("unable to open specified log file: %s",
                            strerror(errno));
-                return NULL;
+                goto failed;
             }
             s->log_file = log_file;
         }
@@ -1323,7 +1323,7 @@ static GAState *initialize_agent(GAConfig *config, int socket_activation)
                                s->pstate_filepath,
                                ga_is_frozen(s))) {
         g_critical("failed to load persistent state");
-        return NULL;
+        goto failed;
     }
 
     config->blacklist = ga_command_blacklist_init(config->blacklist);
@@ -1344,7 +1344,7 @@ static GAState *initialize_agent(GAConfig *config, int socket_activation)
 #ifndef _WIN32
     if (!register_signal_handlers()) {
         g_critical("failed to register signal handlers");
-        return NULL;
+        goto failed;
     }
 #endif
 
@@ -1357,12 +1357,21 @@ static GAState *initialize_agent(GAConfig *config, int socket_activation)
     s->wakeup_event = CreateEvent(NULL, TRUE, FALSE, TEXT("WakeUp"));
     if (s->wakeup_event == NULL) {
         g_critical("CreateEvent failed");
-        return NULL;
+        goto failed;
     }
 #endif
 
     ga_state = s;
     return s;
+
+failed:
+    g_free(s->pstate_filepath);
+    g_free(s->state_filepath_isfrozen);
+    if (s->log_file && s->log_file != stderr) {
+        fclose(s->log_file);
+    }
+    g_free(s);
+    return NULL;
 }
 
 static void cleanup_agent(GAState *s)
-- 
2.31.1
Re: [PATCH] qga: fix possible memory leak
Posted by Markus Armbruster 1 year, 7 months ago 
luzhipeng <luzhipeng@cestc.cn> writes:

> From: lu zhipeng <luzhipeng@cestc.cn>
>
> Signed-off-by: lu zhipeng <luzhipeng@cestc.cn>
> ---
>  qga/main.c | 19 ++++++++++++++-----
>  1 file changed, 14 insertions(+), 5 deletions(-)
>
> diff --git a/qga/main.c b/qga/main.c
> index 5f1efa2333..73ea1aae65 100644
> --- a/qga/main.c
> +++ b/qga/main.c
> @@ -1287,7 +1287,7 @@ static GAState *initialize_agent(GAConfig *config, int socket_activation)
>      if (g_mkdir_with_parents(config->state_dir, S_IRWXU) == -1) {
>          g_critical("unable to create (an ancestor of) the state directory"
>                     " '%s': %s", config->state_dir, strerror(errno));
> -        return NULL;
> +        goto failed;
>      }
>  #endif
>  
> @@ -1312,7 +1312,7 @@ static GAState *initialize_agent(GAConfig *config, int socket_activation)
>              if (!log_file) {
>                  g_critical("unable to open specified log file: %s",
>                             strerror(errno));
> -                return NULL;
> +                goto failed;
>              }
>              s->log_file = log_file;
>          }
> @@ -1323,7 +1323,7 @@ static GAState *initialize_agent(GAConfig *config, int socket_activation)
>                                 s->pstate_filepath,
>                                 ga_is_frozen(s))) {
>          g_critical("failed to load persistent state");
> -        return NULL;
> +        goto failed;
>      }
>  
>      config->blacklist = ga_command_blacklist_init(config->blacklist);
> @@ -1344,7 +1344,7 @@ static GAState *initialize_agent(GAConfig *config, int socket_activation)
>  #ifndef _WIN32
>      if (!register_signal_handlers()) {
>          g_critical("failed to register signal handlers");
> -        return NULL;
> +        goto failed;
>      }
>  #endif
>  
> @@ -1357,12 +1357,21 @@ static GAState *initialize_agent(GAConfig *config, int socket_activation)
>      s->wakeup_event = CreateEvent(NULL, TRUE, FALSE, TEXT("WakeUp"));
>      if (s->wakeup_event == NULL) {
>          g_critical("CreateEvent failed");
> -        return NULL;
> +        goto failed;
>      }
>  #endif
>  
>      ga_state = s;
>      return s;
> +
> +failed:
> +    g_free(s->pstate_filepath);
> +    g_free(s->state_filepath_isfrozen);
> +    if (s->log_file && s->log_file != stderr) {
> +        fclose(s->log_file);
> +    }
> +    g_free(s);
> +    return NULL;
>  }
>  
>  static void cleanup_agent(GAState *s)

The function's only caller is main():

   int main(int argc, char **argv)
   {
       int ret = EXIT_SUCCESS;

       ...

       s = initialize_agent(config, socket_activation);
       if (!s) {
           g_critical("error initializing guest agent");
           goto end;
       }

       ...

   end:
       if (config->daemonize) {
           unlink(config->pid_filepath);
       }

       config_free(config);

       return ret;
   }

When initialize_agent() fails, the process terminates immediately.
There is no memory leak.

We might want to free in initialize_agent() anyway.  Up to the
maintainer.

Bug (not related to this patch): when initialize_agent() fails, we still
terminate successfully.  We should ret = EXIT_FAILURE before goto end,
like we do elsewhere in main().
Re: [PATCH] qga: fix possible memory leak
Posted by Konstantin Kostiuk 1 year, 7 months ago 
On Wed, Sep 21, 2022 at 5:53 PM Markus Armbruster <armbru@redhat.com> wrote:

> luzhipeng <luzhipeng@cestc.cn> writes:
>
> > From: lu zhipeng <luzhipeng@cestc.cn>
> >
> > Signed-off-by: lu zhipeng <luzhipeng@cestc.cn>
> > ---
> >  qga/main.c | 19 ++++++++++++++-----
> >  1 file changed, 14 insertions(+), 5 deletions(-)
> >
> > diff --git a/qga/main.c b/qga/main.c
> > index 5f1efa2333..73ea1aae65 100644
> > --- a/qga/main.c
> > +++ b/qga/main.c
> > @@ -1287,7 +1287,7 @@ static GAState *initialize_agent(GAConfig *config,
> int socket_activation)
> >      if (g_mkdir_with_parents(config->state_dir, S_IRWXU) == -1) {
> >          g_critical("unable to create (an ancestor of) the state
> directory"
> >                     " '%s': %s", config->state_dir, strerror(errno));
> > -        return NULL;
> > +        goto failed;
> >      }
> >  #endif
> >
> > @@ -1312,7 +1312,7 @@ static GAState *initialize_agent(GAConfig *config,
> int socket_activation)
> >              if (!log_file) {
> >                  g_critical("unable to open specified log file: %s",
> >                             strerror(errno));
> > -                return NULL;
> > +                goto failed;
> >              }
> >              s->log_file = log_file;
> >          }
> > @@ -1323,7 +1323,7 @@ static GAState *initialize_agent(GAConfig *config,
> int socket_activation)
> >                                 s->pstate_filepath,
> >                                 ga_is_frozen(s))) {
> >          g_critical("failed to load persistent state");
> > -        return NULL;
> > +        goto failed;
> >      }
> >
> >      config->blacklist = ga_command_blacklist_init(config->blacklist);
> > @@ -1344,7 +1344,7 @@ static GAState *initialize_agent(GAConfig *config,
> int socket_activation)
> >  #ifndef _WIN32
> >      if (!register_signal_handlers()) {
> >          g_critical("failed to register signal handlers");
> > -        return NULL;
> > +        goto failed;
> >      }
> >  #endif
> >
> > @@ -1357,12 +1357,21 @@ static GAState *initialize_agent(GAConfig
> *config, int socket_activation)
> >      s->wakeup_event = CreateEvent(NULL, TRUE, FALSE, TEXT("WakeUp"));
> >      if (s->wakeup_event == NULL) {
> >          g_critical("CreateEvent failed");
> > -        return NULL;
> > +        goto failed;
> >      }
> >  #endif
> >
> >      ga_state = s;
> >      return s;
> > +
> > +failed:
> > +    g_free(s->pstate_filepath);
> > +    g_free(s->state_filepath_isfrozen);
> > +    if (s->log_file && s->log_file != stderr) {
> > +        fclose(s->log_file);
> > +    }
> > +    g_free(s);
>

I think we can just add g_autofree/g_autoptr for all pointers in GAState
and GLib will clean it automatically



> > +    return NULL;
> >  }
> >
> >  static void cleanup_agent(GAState *s)
>
> The function's only caller is main():
>
>    int main(int argc, char **argv)
>    {
>        int ret = EXIT_SUCCESS;
>
>        ...
>
>        s = initialize_agent(config, socket_activation);
>        if (!s) {
>            g_critical("error initializing guest agent");
>            goto end;
>        }
>
>        ...
>
>    end:
>        if (config->daemonize) {
>            unlink(config->pid_filepath);
>        }
>
>        config_free(config);
>
>        return ret;
>    }
>
> When initialize_agent() fails, the process terminates immediately.
> There is no memory leak.
>
> We might want to free in initialize_agent() anyway.  Up to the
> maintainer.
>
> Bug (not related to this patch): when initialize_agent() fails, we still
> terminate successfully.  We should ret = EXIT_FAILURE before goto end,
> like we do elsewhere in main().
>

Yes, I agree with this.

Patchew 2.1-devel-0870f84

© 2016 - 2024 Red Hat, Inc.