Greetings, * Tom Lane (t...@sss.pgh.pa.us) wrote: > Stephen Frost <sfr...@snowman.net> writes: > > * Pavel Raiskup (prais...@redhat.com) 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
From b3551d31f00ac802392aa1982ed0045d4459bbad Mon Sep 17 00:00:00 2001 From: Stephen Frost <sfr...@snowman.net> Date: Wed, 5 Dec 2018 11:53:44 -0500 Subject: [PATCH] Cleanup minor pg_dump memory leaks In dumputils, we may have successfully parsed the acls when we discover that we can't parse the reverse ACLs and then return- check and free aclitems if that happens. In dumpTableSchema, move ftoptions and srvname under the relkind != RELKIND_VIEW branch (since they're only used there) and then check if they've been allocated and, if so, free them at the end of that block. Pointed out by Pavel Raiskup, though I didn't use those patches. Discussion: https://postgr.es/m/2183976.vkcjmhd...@nb.usersys.redhat.com --- src/bin/pg_dump/dumputils.c | 2 ++ src/bin/pg_dump/pg_dump.c | 14 ++++++++------ 2 files changed, 10 insertions(+), 6 deletions(-) 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..637c79af48 100644 --- a/src/bin/pg_dump/pg_dump.c +++ b/src/bin/pg_dump/pg_dump.c @@ -15303,8 +15303,6 @@ dumpTableSchema(Archive *fout, TableInfo *tbinfo) int actual_atts; /* number of attrs in this CREATE statement */ const char *reltypename; char *storage; - char *srvname; - char *ftoptions; int j, k; @@ -15361,6 +15359,9 @@ dumpTableSchema(Archive *fout, TableInfo *tbinfo) } else { + char *ftoptions = NULL; + char *srvname = NULL; + switch (tbinfo->relkind) { case RELKIND_FOREIGN_TABLE: @@ -15397,13 +15398,9 @@ dumpTableSchema(Archive *fout, TableInfo *tbinfo) } case RELKIND_MATVIEW: reltypename = "MATERIALIZED VIEW"; - srvname = NULL; - ftoptions = NULL; break; default: reltypename = "TABLE"; - srvname = NULL; - ftoptions = NULL; } numParents = tbinfo->numParents; @@ -15951,6 +15948,11 @@ dumpTableSchema(Archive *fout, TableInfo *tbinfo) tbinfo->attfdwoptions[j]); } } + + if (ftoptions) + free(ftoptions); + if (srvname) + free(srvname); } /* -- 2.17.1
signature.asc
Description: PGP signature
