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

At Wed, 10 Nov 2021 13:42:25 +0000, PG Bug reporting form <noreply@postgresql.org> wrote in 
> The following bug has been logged on the website:
> 
> Bug reference:      17280
> Logged by:          Alexander Kozhemyakin
> Email address:      a.kozhemyakin@postgrespro.ru
> PostgreSQL version: 14.0
> Operating system:   Ubuntu 21.04
> Description:        
> 
> The following simple query:
> select  * from pg_catalog.pg_stat_slru
> leads to the sanitizer-detected error:
> ==23911==ERROR: AddressSanitizer: global-buffer-overflow on address
> 0x5582bec7c5e0 at pc 0x5582bbd2c01c bp 0x7fff0b73a470 sp 0x7fff0b73a460
> READ of size 64 at 0x5582bec7c5e0 thread T0
>     #0 0x5582bbd2c01b in pg_stat_get_slru
> /home/postgres/postgres/src/backend/utils/adt/pgstatfuncs.c:1914

slruStats has 8 elements (SLRU_NUM_ELEMENTS).

In the loop in pg_stat_get_slru digested as below, 

 >    stats = pgstat_fetch_slru();   - returns slruStats
 >
 >    for (i = 0;; i++)
 >    {
!>        PgStat_SLRUStats stat = stats[i];
 >        name = pgstat_slru_name(i);
 >
 >        if (!name)
 >            break;

The line prefixed by '!' runs with i = 8, which is actually overrun.

I see three ways to fix it. One is to move the assignment to stat to
after the break. Another is to limit the for loop using
SLRU_NUM_ELEMENTS. The last one is limit the for loop using
pgstat_slru_name().

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.

regards.

-- 
Kyotaro Horiguchi
NTT Open Source Software Center
From e29ea453683c8a484cafae24e2a2fa12bf053aee Mon Sep 17 00:00:00 2001
From: Kyotaro Horiguchi <horikyota.ntt@gmail.com>
Date: Thu, 11 Nov 2021 10:10:19 +0900
Subject: [PATCH] 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 ending the loop in a more appropriate way.

Backpatch to 13, where slru stats was introduced.
---
 src/backend/utils/adt/pgstatfuncs.c | 9 ++-------
 1 file changed, 2 insertions(+), 7 deletions(-)

diff --git a/src/backend/utils/adt/pgstatfuncs.c b/src/backend/utils/adt/pgstatfuncs.c
index ff5aedc99c..51fdade9ca 100644
--- a/src/backend/utils/adt/pgstatfuncs.c
+++ b/src/backend/utils/adt/pgstatfuncs.c
@@ -1878,6 +1878,7 @@ pg_stat_get_slru(PG_FUNCTION_ARGS)
     MemoryContext oldcontext;
     int            i;
     PgStat_SLRUStats *stats;
+    const char *name;
 
     /* check to see if caller supports us returning a tuplestore */
     if (rsinfo == NULL || !IsA(rsinfo, ReturnSetInfo))
@@ -1906,18 +1907,12 @@ pg_stat_get_slru(PG_FUNCTION_ARGS)
     /* request SLRU stats from the stat collector */
     stats = pgstat_fetch_slru();
 
-    for (i = 0;; i++)
+    for (i = 0; (name = pgstat_slru_name(i)) != NULL; i++)
     {
         /* for each row */
         Datum        values[PG_STAT_GET_SLRU_COLS];
         bool        nulls[PG_STAT_GET_SLRU_COLS];
         PgStat_SLRUStats stat = stats[i];
-        const char *name;
-
-        name = pgstat_slru_name(i);
-
-        if (!name)
-            break;
 
         MemSet(values, 0, sizeof(values));
         MemSet(nulls, 0, sizeof(nulls));
-- 
2.27.0