Patch a potential memory leak in describeOneTableDetails()

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 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

On 21 Feb 2022, at 09:30, Kyotaro Horiguchi <horikyota.ntt@gmail.com> wrote:

(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)

^~

I think it's because you've indented your new code differently from the
existing, such that the if (res) clause is indented equally to the previous
pg_free(tableinfo.relam) call, making it look like they are in the same block:

+ if (tableinfo.relam)
+ pg_free(tableinfo.relam);

if (res)
PQclear(res);

--
Daniel Gustafsson https://vmware.com/

At Mon, 21 Feb 2022 10:24:23 +0100, Daniel Gustafsson <daniel@yesql.se> wrote in

I think it's because you've indented your new code differently from the
existing, such that the if (res) clause is indented equally to the previous
pg_free(tableinfo.relam) call, making it look like they are in the same block:

Yeah, I (wrongly) thought that they have the same indentation.

+ if (tableinfo.relam)
+ pg_free(tableinfo.relam);

if (res)
PQclear(res);

But actually the lines I added are space-indented but the following
existing line is tab-indented.

Anyway, don't we use the -ftabstop option flag to silence compiler?
The warning is contradicting our convention. The attached adds that
flag.

By the way, the doc says that:

https://www.postgresql.org/docs/14/source-format.html

The text browsing tools more and less can be invoked as:
more -x4
less -x4
to make them show tabs appropriately.

But AFAICS the "more" command on CentOS doesn't have -x options nor
any option to change tab width and I don't find a document that
suggests its existence on other platforms.

regards.

--
Kyotaro Horiguchi
NTT Open Source Software Center

On 22 Feb 2022, at 07:14, Kyotaro Horiguchi <horikyota.ntt@gmail.com> wrote:

Anyway, don't we use the -ftabstop option flag to silence compiler?
The warning is contradicting our convention. The attached adds that
flag.

Isn't this flag contradicting our convention? From the docs section you
reference further down:

"Source code formatting uses 4 column tab spacing, with tabs preserved
(i.e., tabs are not expanded to spaces)."

The -ftabstop option is (to a large extent, not entirely) to warn when tabs and
spaces are mixed creating misleading indentation that the author didn't even
notice due to tabwidth settings? ISTM we are better of getting these warnings
than suppressing them.

By the way, the doc says that:

https://www.postgresql.org/docs/14/source-format.html

The text browsing tools more and less can be invoked as:
more -x4
less -x4
to make them show tabs appropriately.

But AFAICS the "more" command on CentOS doesn't have -x options nor
any option to change tab width and I don't find a document that
suggests its existence on other platforms.

macOS, FreeBSD and NetBSD both show the less(1) manpage for more(1) which
suggests that more is implemented using less there, and thus supports -x (it
does on macOS). OpenBSD and Solaris more(1) does not show a -x option, but AIX
does have it. Linux in its various flavors doesn't seem to have it.

The section in question was added to our docs 22 years ago, to make it reflect
the current operating systems in use maybe just not mentioning more(1) is the
easiest?:

"The text browsing tool less can be invoked as less -x4 to make it show
tabs appropriately."

Or perhaps remove that section altogether?

--
Daniel Gustafsson https://vmware.com/

Kyotaro Horiguchi <horikyota.ntt@gmail.com> writes:

Anyway, don't we use the -ftabstop option flag to silence compiler?
The warning is contradicting our convention. The attached adds that
flag.

No, we use pgindent to not have such inconsistent indentation.

regards, tom lane

At Tue, 22 Feb 2022 09:59:09 +0100, Daniel Gustafsson <daniel@yesql.se> wrote in

On 22 Feb 2022, at 07:14, Kyotaro Horiguchi <horikyota.ntt@gmail.com> wrote:

Anyway, don't we use the -ftabstop option flag to silence compiler?
The warning is contradicting our convention. The attached adds that
flag.

Isn't this flag contradicting our convention? From the docs section you
reference further down:

"Source code formatting uses 4 column tab spacing, with tabs preserved
(i.e., tabs are not expanded to spaces)."

Ugg. Right.

The -ftabstop option is (to a large extent, not entirely) to warn when tabs and
spaces are mixed creating misleading indentation that the author didn't even
notice due to tabwidth settings? ISTM we are better of getting these warnings
than suppressing them.

At Tue, 22 Feb 2022 10:00:46 -0500, Tom Lane <tgl@sss.pgh.pa.us> wrote in

No, we use pgindent to not have such inconsistent indentation.

Understood. Thaks for clarification.

By the way, the doc says that:

https://www.postgresql.org/docs/14/source-format.html

The text browsing tools more and less can be invoked as:
more -x4
less -x4
to make them show tabs appropriately.

But AFAICS the "more" command on CentOS doesn't have -x options nor
any option to change tab width and I don't find a document that
suggests its existence on other platforms.

macOS, FreeBSD and NetBSD both show the less(1) manpage for more(1) which
suggests that more is implemented using less there, and thus supports -x (it
does on macOS). OpenBSD and Solaris more(1) does not show a -x option, but AIX
does have it. Linux in its various flavors doesn't seem to have it.

Thanks for the explanation.

The section in question was added to our docs 22 years ago, to make it reflect
the current operating systems in use maybe just not mentioning more(1) is the
easiest?:

"The text browsing tool less can be invoked as less -x4 to make it show
tabs appropriately."

Or perhaps remove that section altogether?

I think the former is better.

regards.

--
Kyotaro Horiguchi
NTT Open Source Software Center

At Thu, 24 Feb 2022 14:44:56 +0900 (JST), Kyotaro Horiguchi <horikyota.ntt@gmail.com> wrote in

At Tue, 22 Feb 2022 09:59:09 +0100, Daniel Gustafsson <daniel@yesql.se> wrote in

The section in question was added to our docs 22 years ago, to make it reflect
the current operating systems in use maybe just not mentioning more(1) is the
easiest?:

"The text browsing tool less can be invoked as less -x4 to make it show
tabs appropriately."

Or perhaps remove that section altogether?

I think the former is better.

So the attached does that.

regards.

--
Kyotaro Horiguchi
NTT Open Source Software Center

On 24 Feb 2022, at 07:32, Kyotaro Horiguchi <horikyota.ntt@gmail.com> wrote:

At Thu, 24 Feb 2022 14:44:56 +0900 (JST), Kyotaro Horiguchi <horikyota.ntt@gmail.com> wrote in

At Tue, 22 Feb 2022 09:59:09 +0100, Daniel Gustafsson <daniel@yesql.se> wrote in

The section in question was added to our docs 22 years ago, to make it reflect
the current operating systems in use maybe just not mentioning more(1) is the
easiest?:

"The text browsing tool less can be invoked as less -x4 to make it show
tabs appropriately."

Or perhaps remove that section altogether?

I think the former is better.

So the attached does that.

I think this is reasonable, since it's pretty clear that more(1) supporting
"-x" is fairly rare. I'll await others commenting.

--
Daniel Gustafsson https://vmware.com/

On 24 Feb 2022, at 14:55, Daniel Gustafsson <daniel@yesql.se> wrote:

On 24 Feb 2022, at 07:32, Kyotaro Horiguchi <horikyota.ntt@gmail.com> wrote:

So the attached does that.

I think this is reasonable, since it's pretty clear that more(1) supporting
"-x" is fairly rare. I'll await others commenting.

The original patch which prompted this got a bit buried, the attached 0001
contains that fix (using free() which matches the surrounding code) and 0002
has the documentation fixes for the more/less -x command.

--
Daniel Gustafsson https://vmware.com/