Received-SPF: pass (google.com: domain of linux-nfs-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
MIME-Version: 1.0
References: <ee45aa412acaf7a2c035ad98e966394a7293dd9f.camel@redhat.com> <b8b806a99bd53ef9a1e8892f167ea919c52af730.camel@redhat.com>
In-Reply-To: <b8b806a99bd53ef9a1e8892f167ea919c52af730.camel@redhat.com>
From:   Olga Kornievskaia <aglo@umich.edu>
Date:   Fri, 6 Aug 2021 15:12:12 -0400
Message-ID: <CAN-5tyEbTKQg7dNgNQGi0ysiWZnZmOKQgU5u2hy7ejfHoNZxuQ@mail.gmail.com>
Subject: Re: [PATCH 2/4] nfs-utils: Fix mem leaks in gssd
To:     Alice Mitchell <ajmitchell@redhat.com>
Cc:     linux-nfs <linux-nfs@vger.kernel.org>,
        Steve Dickson <steved@redhat.com>
Content-Type: text/plain; charset="UTF-8"
Precedence: bulk

On Fri, Aug 6, 2021 at 12:23 PM Alice Mitchell <ajmitchell@redhat.com> wrote:
>
> Also fix the modification of a returned config value which
> should be treated as const.
>
> Signed-off-by: Alice Mitchell <ajmitchell@redhat.com>
> ---
>  utils/gssd/gssd.c | 10 +++++-----
>  1 file changed, 5 insertions(+), 5 deletions(-)
>
> diff --git a/utils/gssd/gssd.c b/utils/gssd/gssd.c
> index 4113cba..0815665 100644
> --- a/utils/gssd/gssd.c
> +++ b/utils/gssd/gssd.c
> @@ -1016,7 +1016,7 @@ read_gss_conf(void)
>                 keytabfile = s;
>         s = conf_get_str("gssd", "cred-cache-directory");
>         if (s)
> -               ccachedir = s;
> +               ccachedir = strdup(s);
>         s = conf_get_str("gssd", "preferred-realm");
>         if (s)
>                 preferred_realm = s;
> @@ -1070,7 +1070,7 @@ main(int argc, char *argv[])
>                                 keytabfile = optarg;
>                                 break;
>                         case 'd':
> -                               ccachedir = optarg;
> +                               ccachedir = strdup(optarg);

Is it possible that there will be a value in both config file and
command line args. If we are strdup-ing in both we'll be over-writting
and leaking memory?

Why do we need to malloc it at all? Is it ever malloc-ed now (and
considered a leak)? I think in both cases it uses static memory and
doesnt require freeing.

>                                 break;
>                         case 't':
>                                 context_timeout = atoi(optarg);
> @@ -1133,7 +1133,6 @@ main(int argc, char *argv[])
>         }
>
>         if (ccachedir) {
> -               char *ccachedir_copy;
>                 char *ptr;
>
>                 for (ptr = ccachedir, i = 2; *ptr; ptr++)
> @@ -1141,8 +1140,7 @@ main(int argc, char *argv[])
>                                 i++;
>
>                 ccachesearch = malloc(i * sizeof(char *));
> -               ccachedir_copy = strdup(ccachedir);
> -               if (!ccachedir_copy || !ccachesearch) {
> +               if (!ccachesearch) {

ccachedir_copy is the only leak here.

>                         printerr(0, "malloc failure\n");
>                         exit(EXIT_FAILURE);
>                 }
> @@ -1274,6 +1272,8 @@ main(int argc, char *argv[])
>
>         free(preferred_realm);
>         free(ccachesearch);
> +       if (ccachedir)
> +               free(ccachedir);
>
>         return rc < 0 ? EXIT_FAILURE : EXIT_SUCCESS;
>  }
> --
> 2.27.0
>
>