Re: BUG #17280: global-buffer-overflow on select from pg_stat_slru

At Thu, 11 Nov 2021 17:08:34 +0900, Michael Paquier <michael@paquier.xyz> wrote in 
> On Thu, Nov 11, 2021 at 12:19:09PM +0900, Kyotaro Horiguchi wrote:
> > I'm not sure which is easier to read, but it might be a bit hard since
> > the conditino term in not mention counter itself. I don't object to
> > that way.  And, yes SLRU_NUM_ELEMENTS cannot be used here:p
> > 
> > The attached is the first way in the choices.
>
> That looks fine.
> 
> Also, what do you think about adding a test in sysviews.sql? This
> could be as simple as that:
> SELECT count(*) > 0 AS ok FROM pg_stat_slru;

Rahter it is touching only pg_stat_wal and pg_stat_walreceiver among
42 pg_stat_* views. However I agree to it is good to add it for the
reason mentioned below.

> I am not sure if the buildfarm animals running valgrind would have
> caught this issue, but having something would be better than nothing
> for this case.

Valgrind didn't detect that for me (-O0).  The address area accessed
by the overun is allocated to other variables.  That being said.  I
agree that causing access to the full range of the variable should
help checker tools. And that ends in a time shorter than a blink.

Please find the attached.

regrds.

-- 
Kyotaro Horiguchi
NTT Open Source Software Center
From 3c00fc5b8f0b1c855cfe8b1b289275951ee21c48 Mon Sep 17 00:00:00 2001
From: Kyotaro Horiguchi <horikyota.ntt@gmail.com>
Date: Thu, 11 Nov 2021 12:09:45 +0900
Subject: [PATCH v2] Fix memory overrun of pg_stat_get_slru

The function accesses one element after the end of an array, by
accessing the array using a loop variable before exiting a loop.
Avoid that access by accessing the elements after the check against
the exit condition.

The test added by this commit doesn't detect the bug that this commit
fixes on its own. However we add it in the hopes that the same kind of
bugs that future changes could cause will be elicieted for address
sanity checkers.

Backpatch to 13, where slru stats was introduced.
---
 src/backend/utils/adt/pgstatfuncs.c    | 3 ++-
 src/test/regress/expected/sysviews.out | 7 +++++++
 src/test/regress/sql/sysviews.sql      | 3 +++
 3 files changed, 12 insertions(+), 1 deletion(-)

diff --git a/src/backend/utils/adt/pgstatfuncs.c b/src/backend/utils/adt/pgstatfuncs.c
index ff5aedc99c..e64857e540 100644
--- a/src/backend/utils/adt/pgstatfuncs.c
+++ b/src/backend/utils/adt/pgstatfuncs.c
@@ -1911,7 +1911,7 @@ pg_stat_get_slru(PG_FUNCTION_ARGS)
         /* for each row */
         Datum        values[PG_STAT_GET_SLRU_COLS];
         bool        nulls[PG_STAT_GET_SLRU_COLS];
-        PgStat_SLRUStats stat = stats[i];
+        PgStat_SLRUStats stat;
         const char *name;
 
         name = pgstat_slru_name(i);
@@ -1919,6 +1919,7 @@ pg_stat_get_slru(PG_FUNCTION_ARGS)
         if (!name)
             break;
 
+        stat = stats[i];
         MemSet(values, 0, sizeof(values));
         MemSet(nulls, 0, sizeof(nulls));
 
diff --git a/src/test/regress/expected/sysviews.out b/src/test/regress/expected/sysviews.out
index 6e54f3e15e..d9f4243d8b 100644
--- a/src/test/regress/expected/sysviews.out
+++ b/src/test/regress/expected/sysviews.out
@@ -76,6 +76,13 @@ select count(*) >= 0 as ok from pg_prepared_xacts;
  t
 (1 row)
 
+-- There must be several records
+select count(*) > 0 as ok from pg_stat_slru;
+ ok 
+----
+ t
+(1 row)
+
 -- There must be only one record
 select count(*) = 1 as ok from pg_stat_wal;
  ok 
diff --git a/src/test/regress/sql/sysviews.sql b/src/test/regress/sql/sysviews.sql
index dc8c9a3ac2..7fbb8fcdeb 100644
--- a/src/test/regress/sql/sysviews.sql
+++ b/src/test/regress/sql/sysviews.sql
@@ -37,6 +37,9 @@ select count(*) = 0 as ok from pg_prepared_statements;
 -- See also prepared_xacts.sql
 select count(*) >= 0 as ok from pg_prepared_xacts;
 
+-- There must be several records
+select count(*) > 0 as ok from pg_stat_slru;
+
 -- There must be only one record
 select count(*) = 1 as ok from pg_stat_wal;
 
-- 
2.27.0