Thread: BUG #17280: global-buffer-overflow on select from pg_stat_slru
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
#1 0x5582bb405b83 in ExecMakeTableFunctionResult
/home/postgres/postgres/src/backend/executor/execSRF.c:234
#2 0x5582bb45dfd5 in FunctionNext
/home/postgres/postgres/src/backend/executor/nodeFunctionscan.c:95
#3 0x5582bb408a6f in ExecScanFetch
/home/postgres/postgres/src/backend/executor/execScan.c:133
#4 0x5582bb408cba in ExecScan
/home/postgres/postgres/src/backend/executor/execScan.c:182
#5 0x5582bb45db99 in ExecFunctionScan
/home/postgres/postgres/src/backend/executor/nodeFunctionscan.c:270
#6 0x5582bb3fd916 in ExecProcNodeFirst
/home/postgres/postgres/src/backend/executor/execProcnode.c:463
#7 0x5582bb3ddf35 in ExecProcNode
../../../src/include/executor/executor.h:257
#8 0x5582bb3ddf35 in ExecutePlan
/home/postgres/postgres/src/backend/executor/execMain.c:1551
#9 0x5582bb3de54b in standard_ExecutorRun
/home/postgres/postgres/src/backend/executor/execMain.c:361
#10 0x5582bb3de75a in ExecutorRun
/home/postgres/postgres/src/backend/executor/execMain.c:305
#11 0x5582bbabc326 in PortalRunSelect
/home/postgres/postgres/src/backend/tcop/pquery.c:921
#12 0x5582bbac25e3 in PortalRun
/home/postgres/postgres/src/backend/tcop/pquery.c:765
#13 0x5582bbab6277 in exec_simple_query
/home/postgres/postgres/src/backend/tcop/postgres.c:1214
#14 0x5582bbabb2e1 in PostgresMain
/home/postgres/postgres/src/backend/tcop/postgres.c:4497
#15 0x5582bb86dadd in BackendRun
/home/postgres/postgres/src/backend/postmaster/postmaster.c:4584
#16 0x5582bb876e01 in BackendStartup
/home/postgres/postgres/src/backend/postmaster/postmaster.c:4312
#17 0x5582bb8775a9 in ServerLoop
/home/postgres/postgres/src/backend/postmaster/postmaster.c:1801
#18 0x5582bb879d6f in PostmasterMain
/home/postgres/postgres/src/backend/postmaster/postmaster.c:1473
#19 0x5582bb563465 in main
/home/postgres/postgres/src/backend/main/main.c:198
#20 0x7fac88170564 in __libc_start_main
(/lib/x86_64-linux-gnu/libc.so.6+0x28564)
#21 0x5582baba0ded in _start
(/home/postgres/rel_master/bin/postgres+0x1a72ded)
0x5582bec7c5e0 is located 32 bytes to the left of global variable 'walStats'
defined in 'pgstat.c:282:24' (0x5582bec7c600) of size 72
0x5582bec7c5e0 is located 0 bytes to the right of global variable
'slruStats' defined in 'pgstat.c:283:25' (0x5582bec7c3e0) of size 512
SUMMARY: AddressSanitizer: global-buffer-overflow
/home/postgres/postgres/src/backend/utils/adt/pgstatfuncs.c:1914 in
pg_stat_get_slru
Shadow bytes around the buggy address:
0x0ab0d7d87860: f9 f9 f9 f9 00 00 f9 f9 f9 f9 f9 f9 00 f9 f9 f9
0x0ab0d7d87870: f9 f9 f9 f9 00 f9 f9 f9 f9 f9 f9 f9 00 00 00 00
0x0ab0d7d87880: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
0x0ab0d7d87890: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
0x0ab0d7d878a0: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
=>0x0ab0d7d878b0: 00 00 00 00 00 00 00 00 00 00 00 00[f9]f9 f9 f9
0x0ab0d7d878c0: 00 00 00 00 00 00 00 00 00 f9 f9 f9 f9 f9 f9 f9
0x0ab0d7d878d0: 00 00 00 00 00 00 00 00 00 00 00 00 00 f9 f9 f9
0x0ab0d7d878e0: f9 f9 f9 f9 00 00 00 00 00 00 00 00 00 00 00 00
0x0ab0d7d878f0: 00 00 00 00 00 f9 f9 f9 f9 f9 f9 f9 00 f9 f9 f9
0x0ab0d7d87900: f9 f9 f9 f9 00 f9 f9 f9 f9 f9 f9 f9 00 f9 f9 f9
Shadow byte legend (one shadow byte represents 8 application bytes):
Addressable: 00
Partially addressable: 01 02 03 04 05 06 07
Heap left redzone: fa
Freed heap region: fd
Stack left redzone: f1
Stack mid redzone: f2
Stack right redzone: f3
Stack after return: f5
Stack use after scope: f8
Global redzone: f9
Global init order: f6
Poisoned by user: f7
Container overflow: fc
Array cookie: ac
Intra object redzone: bb
ASan internal: fe
Left alloca redzone: ca
Right alloca redzone: cb
Shadow gap: cc
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
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.. -- Michael
Attachment
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
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; 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. -- Michael
Attachment
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
On Fri, Nov 12, 2021 at 04:25:46PM +0900, Kyotaro Horiguchi wrote: > Please find the attached. Thanks, done. -- Michael
Attachment
At Fri, 12 Nov 2021 21:54:14 +0900, Michael Paquier <michael@paquier.xyz> wrote in > On Fri, Nov 12, 2021 at 04:25:46PM +0900, Kyotaro Horiguchi wrote: > > Please find the attached. > > Thanks, done. Thanks for committing! -- Kyotaro Horiguchi NTT Open Source Software Center