Re: minor leaks in pg_dump (PG tarball 10.6) - Mailing list pgsql-hackers

From Stephen Frost
Subject Re: minor leaks in pg_dump (PG tarball 10.6)
Date
Msg-id 20181205155918.GF3415@tamriel.snowman.net
Whole thread Raw
In response to minor leaks in pg_dump (PG tarball 10.6)  (Pavel Raiskup <praiskup@redhat.com>)
Responses Re: minor leaks in pg_dump (PG tarball 10.6)  (Tom Lane <tgl@sss.pgh.pa.us>)
Re: minor leaks in pg_dump (PG tarball 10.6)  (Pavel Raiskup <praiskup@redhat.com>)
List pgsql-hackers
Greetings,

* Pavel Raiskup (praiskup@redhat.com) wrote:
> Among other reports (IMO clearly non-issues), I'm sending patch which
> fixes/points to a few resource leaks detected by Coverity that might be
> worth fixing.  If they are not, feel free to ignore this mail.

> diff --git a/src/bin/pg_dump/dumputils.c b/src/bin/pg_dump/dumputils.c
> index 8a93ace9fa..475d6dbd73 100644
> --- a/src/bin/pg_dump/dumputils.c
> +++ b/src/bin/pg_dump/dumputils.c
> @@ -95,6 +95,8 @@ buildACLCommands(const char *name, const char *subname, const char *nspname,
>      {
>          if (!parsePGArray(racls, &raclitems, &nraclitems))
>          {
> +            if (aclitems)
> +                free(aclitems);
>              if (raclitems)
>                  free(raclitems);
>              return false;

Yeah, that could be fixed.

> diff --git a/src/bin/pg_dump/pg_dump.c b/src/bin/pg_dump/pg_dump.c
> index d583154fba..731a08c15c 100644
> --- a/src/bin/pg_dump/pg_dump.c
> +++ b/src/bin/pg_dump/pg_dump.c
> @@ -8284,7 +8284,6 @@ getTableAttrs(Archive *fout, TableInfo *tblinfo, int numTables)
>              res = ExecuteSqlQuery(fout, q->data, PGRES_TUPLES_OK);
>
>              numDefaults = PQntuples(res);
> -            attrdefs = (AttrDefInfo *) pg_malloc(numDefaults * sizeof(AttrDefInfo));
>
>              for (j = 0; j < numDefaults; j++)
>              {
> @@ -8304,6 +8303,8 @@ getTableAttrs(Archive *fout, TableInfo *tblinfo, int numTables)
>                  if (tbinfo->attisdropped[adnum - 1])
>                      continue;
>
> +                attrdefs = (AttrDefInfo *) pg_malloc(numDefaults * sizeof(AttrDefInfo));
> +

This change doesn't seem to make any sense to me..?  If anything, seems
like we'd end up overallocating memory *after* this change, where we
don't today (though an analyzer tool might complain because we don't
free the memory from it and instead copy the pointer from each of these
items into the tbinfo structure).

Moving the allocation into the loop would also add unnecessary malloc
traffic, so I don't think we should add this.

> @@ -15951,6 +15952,9 @@ dumpTableSchema(Archive *fout, TableInfo *tbinfo)
>                                    tbinfo->attfdwoptions[j]);
>              }
>          }
> +
> +        free(ftoptions);
> +        free(srvname);
>      }

Yes, those could be free'd too.

I'll see about making those changes.

Thanks!

Stephen

Attachment

pgsql-hackers by date:

Previous
From: Pavel Raiskup
Date:
Subject: minor leaks in pg_dump (PG tarball 10.6)
Next
From: Tom Lane
Date:
Subject: Re: minor leaks in pg_dump (PG tarball 10.6)