projects / postgresql.git / commitdiff
summary | shortlog | log | commit | commitdiff | tree
raw | patch | inline | side by side (parent: 76d52e7)
aio: Fix crash potential for pg_aios views due to late state update
authorAndres Freund <andres@anarazel.de>
Fri, 25 Apr 2025 16:03:41 +0000 (12:03 -0400)
committerAndres Freund <andres@anarazel.de>
Fri, 25 Apr 2025 17:31:13 +0000 (13:31 -0400)
pgaio_io_reclaim() reset the fields in PgAioHandle before updating the state
to IDLE or incrementing the generation. For most things that's OK, but for
pg_get_aios() it is not - if it copied the PgAioHandle while fields were being
reset, we wouldn't detect that and could call
pgaio_io_get_target_description() with ioh->target == PGAIO_TID_INVALID,
leading to a crash.

Fix this issue by incrementing the generation and state earlier, before
resetting.

Also add an assertion to pgaio_io_get_target_description() for the target to
be valid - that'd have made this case a bit easier to debug. While at it,
add/update a few related assertions.

Author: Alexander Lakhin <exclusion@gmail.com>
Discussion: https://postgr.es/m/062daca9-dfad-4750-9da8-b13388301ad9@gmail.com

src/backend/storage/aio/aio.c patch | blob | blame | history
src/backend/storage/aio/aio_target.c patch | blob | blame | history

diff --git a/src/backend/storage/aio/aio.c b/src/backend/storage/aio/aio.c
index 64e8e81e18496b4b7755377eca6a7b2b70773a38..e2a133cfac604bfc0f69066b4fda8609baa8c5b4 100644 (file)
--- a/src/backend/storage/aio/aio.c
+++ b/src/backend/storage/aio/aio.c
@@ -670,6 +670,21 @@ pgaio_io_reclaim(PgAioHandle *ioh)
 
    Assert(!ioh->resowner);
 
+   /*
+    * Update generation & state first, before resetting the IO's fields,
+    * otherwise a concurrent "viewer" could think the fields are valid, even
+    * though they are being reset.  Increment the generation first, so that
+    * we can assert elsewhere that we never wait for an IDLE IO.  While it's
+    * a bit weird for the state to go backwards for a generation, it's OK
+    * here, as there cannot be references to the "reborn" IO yet.  Can't
+    * update both at once, so something has to give.
+    */
+   ioh->generation++;
+   pgaio_io_update_state(ioh, PGAIO_HS_IDLE);
+
+   /* ensure the state update is visible before we reset fields */
+   pg_write_barrier();
+
    ioh->op = PGAIO_OP_INVALID;
    ioh->target = PGAIO_TID_INVALID;
    ioh->flags = 0;
@@ -679,12 +694,6 @@ pgaio_io_reclaim(PgAioHandle *ioh)
    ioh->result = 0;
    ioh->distilled_result.status = PGAIO_RS_UNKNOWN;
 
-   /* XXX: the barrier is probably superfluous */
-   pg_write_barrier();
-   ioh->generation++;
-
-   pgaio_io_update_state(ioh, PGAIO_HS_IDLE);
-
    /*
     * We push the IO to the head of the idle IO list, that seems more cache
     * efficient in cases where only a few IOs are used.
diff --git a/src/backend/storage/aio/aio_target.c b/src/backend/storage/aio/aio_target.c
index ac6c74f4ff264cd9322ebb479a59dfbdef3585a8..161f0f1edf68c67864010cab906bf046e49766eb 100644 (file)
--- a/src/backend/storage/aio/aio_target.c
+++ b/src/backend/storage/aio/aio_target.c
@@ -49,7 +49,8 @@ pgaio_io_has_target(PgAioHandle *ioh)
 const char *
 pgaio_io_get_target_name(PgAioHandle *ioh)
 {
-   Assert(ioh->target >= 0 && ioh->target < PGAIO_TID_COUNT);
+   /* explicitly allow INVALID here, function used by debug messages */
+   Assert(ioh->target >= PGAIO_TID_INVALID && ioh->target < PGAIO_TID_COUNT);
 
    return pgaio_target_info[ioh->target]->name;
 }
@@ -82,6 +83,9 @@ pgaio_io_get_target_data(PgAioHandle *ioh)
 char *
 pgaio_io_get_target_description(PgAioHandle *ioh)
 {
+   /* disallow INVALID, there wouldn't be a description */
+   Assert(ioh->target > PGAIO_TID_INVALID && ioh->target < PGAIO_TID_COUNT);
+
    return pgaio_target_info[ioh->target]->describe_identity(&ioh->target_data);
 }
 
@@ -98,6 +102,8 @@ pgaio_io_get_target_description(PgAioHandle *ioh)
 bool
 pgaio_io_can_reopen(PgAioHandle *ioh)
 {
+   Assert(ioh->target > PGAIO_TID_INVALID && ioh->target < PGAIO_TID_COUNT);
+
    return pgaio_target_info[ioh->target]->reopen != NULL;
 }
 
@@ -109,8 +115,8 @@ pgaio_io_can_reopen(PgAioHandle *ioh)
 void
 pgaio_io_reopen(PgAioHandle *ioh)
 {
-   Assert(ioh->target >= 0 && ioh->target < PGAIO_TID_COUNT);
-   Assert(ioh->op >= 0 && ioh->op < PGAIO_OP_COUNT);
+   Assert(ioh->target > PGAIO_TID_INVALID && ioh->target < PGAIO_TID_COUNT);
+   Assert(ioh->op > PGAIO_OP_INVALID && ioh->op < PGAIO_OP_COUNT);
 
    pgaio_target_info[ioh->target]->reopen(ioh);
 }
This is the main PostgreSQL git repository.