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

At Thu, 11 Nov 2021 11:52:27 +0900, Michael Paquier <michael@paquier.xyz> wrote in 
> On Thu, Nov 11, 2021 at 10:39:23AM +0900, Kyotaro Horiguchi wrote:
> > The loop is designed not to directly rely on SRLU_NUM_ELEMENTS so if
> > we honor that design, we would take the first or the third way. The
> > first way is smallest but I prefer the third way as it is
> > straightforward as such kind of loops.  The attached is that for the
> > master.
> > 
> > The code was introduced at 13 and the attached applies to the versions
> > back to 13.
> 
> Or it would be easier for the reader to assign stat after checking for
> the result of pgstat_slru_name(), no?  I am not much a fan of this
> code style that uses a counter, FWIW, but at the same time
> SLRU_NUM_ELEMENTS is local to pgstat.c, so..

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.

regards.

-- 
Kyotaro Horiguchi
NTT Open Source Software Center
From 6ddc188713f6314e25a78ec4c4f095376e6263c6 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.

Backpatch to 13, where slru stats was introduced.
---
 src/backend/utils/adt/pgstatfuncs.c | 3 ++-
 1 file changed, 2 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));
 
-- 
2.27.0