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

> 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
From 4e3b0c2ce6a2dd60d11100deb2db80ff07455d0d Mon Sep 17 00:00:00 2001
From: Kyotaro Horiguchi <horikyota.ntt@gmail.com>
Date: Tue, 22 Feb 2022 14:41:10 +0900
Subject: [PATCH] Fit compiler tabstop to our convension

Set -ftabstop to 4 so that -Wmisleading-indentation doesn't makes
false warnings.
---
 configure    | 92 ++++++++++++++++++++++++++++++++++++++++++++++++++++
 configure.ac |  3 ++
 2 files changed, 95 insertions(+)

diff --git a/configure b/configure
index f3cb5c2b51..887ed733eb 100755
--- a/configure
+++ b/configure
@@ -6219,6 +6219,98 @@ if test x"$pgac_cv_prog_CXX_cxxflags__fexcess_precision_standard" = x"yes"; then
 fi
 
 
+  # Fit tab width for -Wmisleading-indentation to our convention
+
+{ $as_echo "$as_me:${as_lineno-$LINENO}: checking whether ${CC} supports -ftabstop=4, for CFLAGS" >&5
+$as_echo_n "checking whether ${CC} supports -ftabstop=4, for CFLAGS... " >&6; }
+if ${pgac_cv_prog_CC_cflags__ftabstop_4+:} false; then :
+  $as_echo_n "(cached) " >&6
+else
+  pgac_save_CFLAGS=$CFLAGS
+pgac_save_CC=$CC
+CC=${CC}
+CFLAGS="${CFLAGS} -ftabstop=4"
+ac_save_c_werror_flag=$ac_c_werror_flag
+ac_c_werror_flag=yes
+cat confdefs.h - <<_ACEOF >conftest.$ac_ext
+/* end confdefs.h.  */
+
+int
+main ()
+{
+
+  ;
+  return 0;
+}
+_ACEOF
+if ac_fn_c_try_compile "$LINENO"; then :
+  pgac_cv_prog_CC_cflags__ftabstop_4=yes
+else
+  pgac_cv_prog_CC_cflags__ftabstop_4=no
+fi
+rm -f core conftest.err conftest.$ac_objext conftest.$ac_ext
+ac_c_werror_flag=$ac_save_c_werror_flag
+CFLAGS="$pgac_save_CFLAGS"
+CC="$pgac_save_CC"
+fi
+{ $as_echo "$as_me:${as_lineno-$LINENO}: result: $pgac_cv_prog_CC_cflags__ftabstop_4" >&5
+$as_echo "$pgac_cv_prog_CC_cflags__ftabstop_4" >&6; }
+if test x"$pgac_cv_prog_CC_cflags__ftabstop_4" = x"yes"; then
+  CFLAGS="${CFLAGS} -ftabstop=4"
+fi
+
+
+  { $as_echo "$as_me:${as_lineno-$LINENO}: checking whether ${CXX} supports -ftabstop=4, for CXXFLAGS" >&5
+$as_echo_n "checking whether ${CXX} supports -ftabstop=4, for CXXFLAGS... " >&6; }
+if ${pgac_cv_prog_CXX_cxxflags__ftabstop_4+:} false; then :
+  $as_echo_n "(cached) " >&6
+else
+  pgac_save_CXXFLAGS=$CXXFLAGS
+pgac_save_CXX=$CXX
+CXX=${CXX}
+CXXFLAGS="${CXXFLAGS} -ftabstop=4"
+ac_save_cxx_werror_flag=$ac_cxx_werror_flag
+ac_cxx_werror_flag=yes
+ac_ext=cpp
+ac_cpp='$CXXCPP $CPPFLAGS'
+ac_compile='$CXX -c $CXXFLAGS $CPPFLAGS conftest.$ac_ext >&5'
+ac_link='$CXX -o conftest$ac_exeext $CXXFLAGS $CPPFLAGS $LDFLAGS conftest.$ac_ext $LIBS >&5'
+ac_compiler_gnu=$ac_cv_cxx_compiler_gnu
+
+cat confdefs.h - <<_ACEOF >conftest.$ac_ext
+/* end confdefs.h.  */
+
+int
+main ()
+{
+
+  ;
+  return 0;
+}
+_ACEOF
+if ac_fn_cxx_try_compile "$LINENO"; then :
+  pgac_cv_prog_CXX_cxxflags__ftabstop_4=yes
+else
+  pgac_cv_prog_CXX_cxxflags__ftabstop_4=no
+fi
+rm -f core conftest.err conftest.$ac_objext conftest.$ac_ext
+ac_ext=c
+ac_cpp='$CPP $CPPFLAGS'
+ac_compile='$CC -c $CFLAGS $CPPFLAGS conftest.$ac_ext >&5'
+ac_link='$CC -o conftest$ac_exeext $CFLAGS $CPPFLAGS $LDFLAGS conftest.$ac_ext $LIBS >&5'
+ac_compiler_gnu=$ac_cv_c_compiler_gnu
+
+ac_cxx_werror_flag=$ac_save_cxx_werror_flag
+CXXFLAGS="$pgac_save_CXXFLAGS"
+CXX="$pgac_save_CXX"
+fi
+{ $as_echo "$as_me:${as_lineno-$LINENO}: result: $pgac_cv_prog_CXX_cxxflags__ftabstop_4" >&5
+$as_echo "$pgac_cv_prog_CXX_cxxflags__ftabstop_4" >&6; }
+if test x"$pgac_cv_prog_CXX_cxxflags__ftabstop_4" = x"yes"; then
+  CXXFLAGS="${CXXFLAGS} -ftabstop=4"
+fi
+
+
   # Optimization flags for specific files that benefit from loop unrolling
   { $as_echo "$as_me:${as_lineno-$LINENO}: checking whether ${CC} supports -funroll-loops, for CFLAGS_UNROLL_LOOPS"
>&5
 $as_echo_n "checking whether ${CC} supports -funroll-loops, for CFLAGS_UNROLL_LOOPS... " >&6; }
diff --git a/configure.ac b/configure.ac
index 19d1a80367..808d3d268d 100644
--- a/configure.ac
+++ b/configure.ac
@@ -518,6 +518,9 @@ if test "$GCC" = yes -a "$ICC" = no; then
   # Disable FP optimizations that cause various errors on gcc 4.5+ or maybe 4.6+
   PGAC_PROG_CC_CFLAGS_OPT([-fexcess-precision=standard])
   PGAC_PROG_CXX_CFLAGS_OPT([-fexcess-precision=standard])
+  # Fit tab width for -Wmisleading-indentation to our convention
+  PGAC_PROG_CC_CFLAGS_OPT([-ftabstop=4])
+  PGAC_PROG_CXX_CFLAGS_OPT([-ftabstop=4])
   # Optimization flags for specific files that benefit from loop unrolling
   PGAC_PROG_CC_VAR_OPT(CFLAGS_UNROLL_LOOPS, [-funroll-loops])
   # Optimization flags for specific files that benefit from vectorization
-- 
2.27.0

> 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
From fd833aee1756364f669a3d478763299ce6d3a747 Mon Sep 17 00:00:00 2001
From: Kyotaro Horiguchi <horikyota.ntt@gmail.com>
Date: Thu, 24 Feb 2022 15:18:52 +0900
Subject: [PATCH] Fix out-of-date description mentioning the command more

In most of the operating systems in use, "more" is implemented using
"less", or otherwise "more" does not have an -x option. Remove the
descriptions related to the more command to make it reflect the
current world better.
---
 doc/src/sgml/sources.sgml | 9 ++-------
 1 file changed, 2 insertions(+), 7 deletions(-)

diff --git a/doc/src/sgml/sources.sgml b/doc/src/sgml/sources.sgml
index 1b77efb087..7a6e8a951f 100644
--- a/doc/src/sgml/sources.sgml
+++ b/doc/src/sgml/sources.sgml
@@ -76,13 +76,8 @@
    </para>
 
    <para>
-    The text browsing tools <application>more</application> and
-    <application>less</application> can be invoked as:
-<programlisting>
-more -x4
-less -x4
-</programlisting>
-    to make them show tabs appropriately.
+    The text browsing tool <application>less</application> can be invoked
+    as <quote>less -x4</quote> to make it show tabs appropriately.
    </para>
   </sect1>
 
-- 
2.27.0

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