Re: minor leaks in pg_dump (PG tarball 10.6)

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