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

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

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

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

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

On Fri, Nov 12, 2021 at 04:25:46PM +0900, Kyotaro Horiguchi wrote:

Please find the attached.

Thanks, done.
--
Michael

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