Re: BUG #17280: global-buffer-overflow on select from pg_stat_slru - Mailing list pgsql-bugs
From | Kyotaro Horiguchi |
---|---|
Subject | Re: BUG #17280: global-buffer-overflow on select from pg_stat_slru |
Date | |
Msg-id | 20211112.162546.804840249267031436.horikyota.ntt@gmail.com Whole thread Raw |
In response to | Re: BUG #17280: global-buffer-overflow on select from pg_stat_slru (Michael Paquier <michael@paquier.xyz>) |
Responses |
Re: BUG #17280: global-buffer-overflow on select from pg_stat_slru
|
List | pgsql-bugs |
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
pgsql-bugs by date: