At Fri, 18 Feb 2022 16:12:34 +0800 (GMT+08:00), wli...@stu.xidian.edu.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

Reply via email to