At Fri, 18 Feb 2022 16:12:34 +0800 (GMT+08:00), [email protected] 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
