Re: Patch a potential memory leak in describeOneTableDetails() - Mailing list pgsql-hackers

At Fri, 18 Feb 2022 16:12:34 +0800 (GMT+08:00), wliang@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
branchtakes true at line 1621, the execution path will eventually jump to error_return at line 3394. Unfortunately, the
memoryis 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
alsobe 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