Re: minor leaks in pg_dump (PG tarball 10.6)

From: Stephen Frost <sfrost(at)snowman(dot)net>
To: Pavel Raiskup <praiskup(at)redhat(dot)com>
Cc: pgsql-hackers(at)lists(dot)postgresql(dot)org
Subject: Re: minor leaks in pg_dump (PG tarball 10.6)
Date: 2018-12-05 15:59:18
Message-ID: 20181205155918.GF3415@tamriel.snowman.net
Views: Raw Message | Whole Thread | Download mbox | Resend email
Thread:
Lists: pgsql-hackers

Greetings,

* Pavel Raiskup (praiskup(at)redhat(dot)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

In response to

Responses

Browse pgsql-hackers by date

  From Date Subject
Next Message Tom Lane 2018-12-05 16:06:48 Re: minor leaks in pg_dump (PG tarball 10.6)
Previous Message Pavel Raiskup 2018-12-05 15:43:39 minor leaks in pg_dump (PG tarball 10.6)