Re: Plug minor memleak in pg_dump

At Tue, 1 Feb 2022 19:48:01 +0530, Bharath Rupireddy <bharath(dot)rupireddyforpostgres(at)gmail(dot)com> wrote in
> On Tue, Feb 1, 2022 at 7:06 PM <gkokolatos(at)pm(dot)me> wrote:
> >
> > Hi,
> >
> > I noticed a minor memleak in pg_dump. ReadStr() returns a malloc'ed pointer which
> > should then be freed. While reading the Table of Contents, it was called as an argument
> > within a function call, leading to a memleak.
> >
> > Please accept the attached as a proposed fix.

It is freed in other temporary use of the result of ReadStr(). So
freeing it sounds sensible at a glance.

> +1. IMO, having "restoring tables WITH OIDS is not supported anymore"
> twice doesn't look good, how about as shown in [1]?

Maybe [2] is smaller :)

--- a/src/bin/pg_dump/pg_backup_archiver.c
+++ b/src/bin/pg_dump/pg_backup_archiver.c
@@ -2494,6 +2494,7 @@ ReadToc(ArchiveHandle *AH)
int depIdx;
int depSize;
TocEntry *te;
+ char *tmpstr = NULL;

AH->tocCount = ReadInt(AH);
AH->maxDumpId = 0;
@@ -2574,8 +2575,14 @@ ReadToc(ArchiveHandle *AH)
te->tableam = ReadStr(AH);

te->owner = ReadStr(AH);
- if (AH->version < K_VERS_1_9 || strcmp(ReadStr(AH), "true") == 0)
+ if (AH->version < K_VERS_1_9 ||
+ strcmp((tmpstr = ReadStr(AH)), "true") == 0)
pg_log_warning("restoring tables WITH OIDS is not supported anymore");
+ if (tmpstr)
+ {
+ pg_free(tmpstr);
+ tmpstr = NULL;
+ }

/* Read TOC entry dependencies */

Thus.. I came to doubt of its worthiness to the complexity. The
amount of the leak is (perhaps) negligible.

So, I would just write a comment there.

+++ b/src/bin/pg_dump/pg_backup_archiver.c
@@ -2574,6 +2574,8 @@ ReadToc(ArchiveHandle *AH)
te->tableam = ReadStr(AH);

te->owner = ReadStr(AH);
+
+ /* deliberately leak the result of ReadStr for simplicity */
if (AH->version < K_VERS_1_9 || strcmp(ReadStr(AH), "true") == 0)
pg_log_warning("restoring tables WITH OIDS is not supported anymore");

regards.

--
Kyotaro Horiguchi
NTT Open Source Software Center