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;
Message-ID: <76b96fa95dfb6c53f28cae6b622f71559d8bfe6c.camel@redhat.com>
Subject: Re: [PATCH 2/4] nfs-utils: Fix mem leaks in gssd
From:   Alice Mitchell <ajmitchell@redhat.com>
To:     Olga Kornievskaia <aglo@umich.edu>
Cc:     linux-nfs <linux-nfs@vger.kernel.org>,
        Steve Dickson <steved@redhat.com>
Date:   Thu, 12 Aug 2021 14:19:21 +0100
In-Reply-To: <CAN-5tyEbTKQg7dNgNQGi0ysiWZnZmOKQgU5u2hy7ejfHoNZxuQ@mail.gmail.com>
References: <ee45aa412acaf7a2c035ad98e966394a7293dd9f.camel@redhat.com>
         <b8b806a99bd53ef9a1e8892f167ea919c52af730.camel@redhat.com>
         <CAN-5tyEbTKQg7dNgNQGi0ysiWZnZmOKQgU5u2hy7ejfHoNZxuQ@mail.gmail.com>
Content-Type: text/plain; charset="UTF-8"
Mime-Version: 1.0
Content-Transfer-Encoding: 7bit
Precedence: bulk

On Fri, 2021-08-06 at 15:12 -0400, Olga Kornievskaia wrote:
> 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.

in both cases the string pointed to gets modified by a later strtok()
and so will be modifying the original of argv or a conf parameter,
which i presume is why there was ccacherdir_copy to avoid doing that,
but it was never properly utilised.

i guess strtok truncating those strings doesnt actually *hurt* anything
right now, its just a case of unexpected side-effects should anyone
later try to reuse them.

> 
> >                                 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
> > 
> >