[PATCH 0/4] nfs-utils: A series of memory fixes

[PATCH 0/4] nfs-utils: A series of memory fixes

[Alice Mitchell]

This series of patches fix a number of potential memory leaks
and memory errors within nfs-utils that mostly happen under
various error conditions.

Signed-off-by: Alice Mitchell <ajmitchell@redhat.com>

Alice Mitchell (4):
  nfs-utils: Fix potential memory leaks in idmap
  nfs-utils: Fix mem leaks in gssd
  nfs-utils: Fix mem leaks in krb5_util
  nfs-utils: Fix mem leak in mountd

 support/nfsidmap/nss.c   |  4 ++--
 support/nfsidmap/regex.c |  1 +
 utils/gssd/gssd.c        | 10 +++++-----
 utils/gssd/krb5_util.c   | 18 ++++++++++++++++--
 utils/mountd/rmtab.c     |  3 +++
 5 files changed, 27 insertions(+), 9 deletions(-)

-- 
2.27.0

[PATCH 1/4] nfs-utils: Fix potential memory leaks in idmap

[Alice Mitchell]

Signed-off-by: Alice Mitchell <ajmitchell@redhat.com>
---
 support/nfsidmap/nss.c   | 4 ++--
 support/nfsidmap/regex.c | 1 +
 2 files changed, 3 insertions(+), 2 deletions(-)

diff --git a/support/nfsidmap/nss.c b/support/nfsidmap/nss.c
index 669760b..f00bd9b 100644
--- a/support/nfsidmap/nss.c
+++ b/support/nfsidmap/nss.c
@@ -365,9 +365,9 @@ static int _nss_name_to_gid(char *name, gid_t *gid,
int dostrip)
 out_buf:
 	free(buf);
 out_name:
-	if (dostrip)
+	if (localname)
 		free(localname);
-	if (get_reformat_group())
+	if (ref_name)
 		free(ref_name);
 out:
 	return err;
diff --git a/support/nfsidmap/regex.c b/support/nfsidmap/regex.c
index fdbb2e2..958b4ac 100644
--- a/support/nfsidmap/regex.c
+++ b/support/nfsidmap/regex.c
@@ -157,6 +157,7 @@ again:
 	IDMAP_LOG(4, ("regexp_getpwnam: name '%s' mapped to '%s'",
 		  name, localname));
 
+	free(localname);
 	*err_p = 0;
 	return pw;
 
-- 
2.27.0

Re: [PATCH 1/4] nfs-utils: Fix potential memory leaks in idmap

[Olga Kornievskaia]

On Fri, Aug 6, 2021 at 12:21 PM Alice Mitchell <ajmitchell@redhat.com> wrote:
>
>
> Signed-off-by: Alice Mitchell <ajmitchell@redhat.com>
> ---
>  support/nfsidmap/nss.c   | 4 ++--
>  support/nfsidmap/regex.c | 1 +
>  2 files changed, 3 insertions(+), 2 deletions(-)
>
> diff --git a/support/nfsidmap/nss.c b/support/nfsidmap/nss.c
> index 669760b..f00bd9b 100644
> --- a/support/nfsidmap/nss.c
> +++ b/support/nfsidmap/nss.c
> @@ -365,9 +365,9 @@ static int _nss_name_to_gid(char *name, gid_t *gid,
> int dostrip)
>  out_buf:
>         free(buf);
>  out_name:
> -       if (dostrip)
> +       if (localname)
>                 free(localname);
> -       if (get_reformat_group())
> +       if (ref_name)
>                 free(ref_name);

Do we even need to check for null before freeing these days? man page
says if null is passed then it's a no-op.

If we are not allowed to free a null then there is another patch in
the series in the mountd code that does intentionally do a free of
null pointers.

>  out:
>         return err;
> diff --git a/support/nfsidmap/regex.c b/support/nfsidmap/regex.c
> index fdbb2e2..958b4ac 100644
> --- a/support/nfsidmap/regex.c
> +++ b/support/nfsidmap/regex.c
> @@ -157,6 +157,7 @@ again:
>         IDMAP_LOG(4, ("regexp_getpwnam: name '%s' mapped to '%s'",
>                   name, localname));
>
> +       free(localname);
>         *err_p = 0;
>         return pw;
>
> --
> 2.27.0
>
>

[PATCH 2/4] nfs-utils: Fix mem leaks in gssd

[Alice Mitchell]

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);
 				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) {
 			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

Re: [PATCH 2/4] nfs-utils: Fix mem leaks in gssd

[Olga Kornievskaia]

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

Re: [PATCH 2/4] nfs-utils: Fix mem leaks in gssd

[Alice Mitchell]

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

[PATCH 3/4] nfs-utils: Fix mem leaks in krb5_util

[Alice Mitchell]

Re: [PATCH 3/4] nfs-utils: Fix mem leaks in krb5_util

[Olga Kornievskaia]

Empty patch?

On Fri, Aug 6, 2021 at 12:25 PM Alice Mitchell <ajmitchell@redhat.com> wrote:
>
>

[PATCH 4/4] nfs-utils: Fix mem leak in mountd

[Alice Mitchell]

Signed-off-by: Alice Mitchell <ajmitchell@redhat.com>
---
 utils/mountd/rmtab.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/utils/mountd/rmtab.c b/utils/mountd/rmtab.c
index 2da9761..752fdb6 100644
--- a/utils/mountd/rmtab.c
+++ b/utils/mountd/rmtab.c
@@ -233,6 +233,9 @@ mountlist_list(void)
 			m->ml_directory = strdup(rep->r_path);
 
 			if (m->ml_hostname == NULL || m->ml_directory == NULL) {
+				free(m->ml_hostname);
+				free(m->ml_directory);
+				free(m);
 				mountlist_freeall(mlist);
 				mlist = NULL;
 				xlog(L_ERROR, "%s: memory allocation failed",
-- 
2.27.0

[PATCH 3/4] nfs-utils: Fix mem leaks in krb5_util

[Alice Mitchell]

Re: [PATCH 3/4] nfs-utils: Fix mem leaks in krb5_util

[Olga Kornievskaia]

Still empty for me... :-/

On Fri, Aug 6, 2021 at 2:50 PM Alice Mitchell <ajmitchell@redhat.com> wrote:
>
>