Re: Patch a potential memory leak in describeOneTableDetails()

At Fri, 18 Feb 2022 16:12:34 +0800 (GMT+08:00), wliang(at)stu(dot)xidian(dot)edu(dot)cn wrote in
> I find a potential memory leak in PostgresSQL 14.1, which is in the function describeOneTableDetails (./src/bin/psql/describe.c). The bug has been confirmed by an auditor of <pgsql-bugs>.
>
> Specifically, at line 1603, a memory chunk is allocated with pg_strdup and assigned to tableinfo.reloptions. If the branch takes true at line 1621, the execution path will eventually jump to error_return at line 3394. Unfortunately, the memory is not freed when the function describeOneTableDetail() returns. As a result, the memory is leaked.
>
> Similarly, the two memory chunks (tableinfo.reloftype and tableinfo.relam) allocated at line 1605, 1606 and 1612 may also be leaked for the same reason.

I think it is not potential leaks but real leaks as it accumulates as repeatedly doing \d <table>.

> We believe we can fix the problems by adding corresponding release function before the function returns, such as.
>
> if (tableinfo.reloptions)
> pg_free (tableinfo.reloptions);
> if (tableinfo.reloftype)
> pg_free (tableinfo.reloftype);
> if (tableinfo.relam)
> pg_free (tableinfo. relam);
>
>
> I'm looking forward to your reply.

Good catch, and the fix is logically correct. The results from other
use of pg_strdup() (and pg_malloc) seems to be released properly.

About the patch, we should make a single chunk to do free(). And I
think we don't insert whitespace between function name and the
following left parenthesis.

Since view_def is allocated by pg_strdup(), we might be better use
pg_free() for it for symmetry. footers[0] is allocated using the
frontend-alternative of "palloc()" so should use pfree() instead?

Honestly I'm not sure we need to be so strict on that
correspondence...

So it would be something like this.

diff --git a/src/bin/psql/describe.c b/src/bin/psql/describe.c
index 654ef2d7c3..5da2b61a88 100644
--- a/src/bin/psql/describe.c
+++ b/src/bin/psql/describe.c
@@ -3412,7 +3412,13 @@ error_return:
termPQExpBuffer(&tmpbuf);

if (view_def)
- free(view_def);
+ pg_free(view_def);
+ if (tableinfo.reloptions)
+ pg_free(tableinfo.reloptions);
+ if (tableinfo.reloftype)
+ pg_free(tableinfo.reloftype);
+ if (tableinfo.relam)
+ pg_free(tableinfo.relam);

if (res)
PQclear(res);
@@ -3621,8 +3627,8 @@ describeRoles(const char *pattern, bool verbose, bool showSystem)
printTableCleanup(&cont);

for (i = 0; i < nrows; i++)
- free(attr[i]);
- free(attr);
+ pg_free(attr[i]);
+ pg_free(attr);

PQclear(res);
return true;

(However, I got a mysterious -Wmisleading-indentation warning with this..)

> describe.c: In function ‘describeOneTableDetails’:
> describe.c:3420:5: warning: this ‘if’ clause does not guard... [-Wmisleading-indentation]
> if (tableinfo.relam)
> ^~
> describe.c:3423:2: note: ...this statement, but the latter is misleadingly indented as if it were guarded by the ‘if’
> if (res)
^~

regards.

--
Kyotaro Horiguchi
NTT Open Source Software Center