minor leaks in pg_dump (PG tarball 10.6)

classic Classic list List threaded Threaded
6 messages Options
Reply | Threaded
Open this post in threaded view
|

minor leaks in pg_dump (PG tarball 10.6)

Pavel Raiskup
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.

Pavel

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;
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));
+
  attrdefs[j].dobj.objType = DO_ATTRDEF;
  attrdefs[j].dobj.catId.tableoid = atooid(PQgetvalue(res, j, 0));
  attrdefs[j].dobj.catId.oid = atooid(PQgetvalue(res, j, 1));
@@ -15951,6 +15952,9 @@ dumpTableSchema(Archive *fout, TableInfo *tbinfo)
   tbinfo->attfdwoptions[j]);
  }
  }
+
+ free(ftoptions);
+ free(srvname);
  }
 
  /*




Reply | Threaded
Open this post in threaded view
|

Re: minor leaks in pg_dump (PG tarball 10.6)

Stephen Frost
Greetings,

* Pavel Raiskup ([hidden email]) 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

signature.asc (836 bytes) Download Attachment
Reply | Threaded
Open this post in threaded view
|

Re: minor leaks in pg_dump (PG tarball 10.6)

Tom Lane-2
Stephen Frost <[hidden email]> writes:
> * Pavel Raiskup ([hidden email]) wrote:
>> - attrdefs = (AttrDefInfo *) pg_malloc(numDefaults * sizeof(AttrDefInfo));
>> ...
>> + 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).

Yeah, Coverity is exceedingly not smart about the method pg_dump uses
(in lots of places, not just here) of allocating an array and then
entering pointers to individual array elements into its long-lived
data structures.  I concur that the proposed change is giving up a
lot of malloc overhead to silence an invalid complaint, and we
shouldn't do it.

The other two points seem probably valid, so I wonder why our own
Coverity runs haven't noticed them.

                        regards, tom lane

Reply | Threaded
Open this post in threaded view
|

Re: minor leaks in pg_dump (PG tarball 10.6)

Stephen Frost
Greetings,

* Tom Lane ([hidden email]) wrote:

> Stephen Frost <[hidden email]> writes:
> > * Pavel Raiskup ([hidden email]) wrote:
> >> - attrdefs = (AttrDefInfo *) pg_malloc(numDefaults * sizeof(AttrDefInfo));
> >> ...
> >> + 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).
>
> Yeah, Coverity is exceedingly not smart about the method pg_dump uses
> (in lots of places, not just here) of allocating an array and then
> entering pointers to individual array elements into its long-lived
> data structures.  I concur that the proposed change is giving up a
> lot of malloc overhead to silence an invalid complaint, and we
> shouldn't do it.
Agreed, patch attached.  If there aren't any more comments, I'll plan to
push this later today or tomorrow.  I wasn't thinking we would backpatch
this, so if others feel differently, please let me know.

> The other two points seem probably valid, so I wonder why our own
> Coverity runs haven't noticed them.

Not sure..  Looks like Coverity (incorrectly) worries about srvname
being NULL and then dereferenced inside fmtId(), except that relkind
doesn't change between the switch() and the conditional where srvname is
used.  Maybe that is masking the resource leak concern?  I don't see it
complaining about ftoptions though, so really not sure what's going on
there.  I can try to ask the Coverity admin folks, but they've yet to
respond to multiple requests I've made about linking in the v11 branch
with the others..

Thanks!

Stephen

fix-pg_dump-memory-leak_v1.patch (2K) Download Attachment
signature.asc (836 bytes) Download Attachment
Reply | Threaded
Open this post in threaded view
|

Re: minor leaks in pg_dump (PG tarball 10.6)

Pavel Raiskup
In reply to this post by Stephen Frost
On Wednesday, December 5, 2018 4:59:18 PM CET Stephen Frost wrote:
> 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).

Correct, I haven't think that one through.  I was confused that some items
related to the dropped columns could be unreferenced.  But those are
anyways allocated as a solid block with others (not intended to be ever
free()'d).  Feel free to ignore that.

Thanks for looking at this!
Pavel




Reply | Threaded
Open this post in threaded view
|

Re: minor leaks in pg_dump (PG tarball 10.6)

Stephen Frost
Greetings,

* Pavel Raiskup ([hidden email]) wrote:

> On Wednesday, December 5, 2018 4:59:18 PM CET Stephen Frost wrote:
> > 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).
>
> Correct, I haven't think that one through.  I was confused that some items
> related to the dropped columns could be unreferenced.  But those are
> anyways allocated as a solid block with others (not intended to be ever
> free()'d).  Feel free to ignore that.
Right.

I've pushed the other changes.

Thanks!

Stephen

signature.asc (836 bytes) Download Attachment