Re: [PATCH] qga: fix possible memory leak

[Markus Armbruster <armbru@redhat.com>]

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().