Обсуждение: 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
Вложения
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
Вложения
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
Вложения
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