On Fri, 2021-08-06 at 15:12 -0400, Olga Kornievskaia wrote:
> On Fri, Aug 6, 2021 at 12:23 PM Alice Mitchell <ajmitchell@xxxxxxxxxx
> > wrote:
> > Also fix the modification of a returned config value which
> > should be treated as const.
> >
> > Signed-off-by: Alice Mitchell <ajmitchell@xxxxxxxxxx>
> > ---
> > 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
> >
> >
