Use-after-free issue in postgres_fdw

On Mar 19, 2026, at 22:56, Etsuro Fujita <etsuro.fujita@gmail.com> wrote:

Hi,

I got an offline report from my colleague Zhibai Song that
close_cursor() is called for a freed PGconn, leading to a server
crash. Here is a reproducer (the original reproducer he provided is a
bit complex, so I simplified it):

create server loopback
foreign data wrapper postgres_fdw
options (dbname 'postgres');
create user mapping for current_user server loopback;
create table t1 (id int, data text);
create foreign table ft1 (id int, data text)
server loopback options (table_name 't1');
insert into ft1 values (1, 'foo');
start transaction;
-- This caches the remote connection's PGconn in PgFdwScanState
declare c1 cursor for select * from ft1;
fetch c1;
id | data
----+------
1 | foo
(1 row)

savepoint s1;
select * from ft1;
id | data
----+------
1 | foo
(1 row)

select pid from pg_stat_activity
where datname = 'postgres'
and application_name = 'postgres_fdw';
pid
-------
91853
(1 row)

-- This terminates the remote session
select pg_terminate_backend(91853);
pg_terminate_backend
----------------------
t
(1 row)

-- This leaves the remote connection's changing_xact_state as true
rollback to s1;
savepoint s1;
-- This calls pgfdw_reject_incomplete_xact_state_change(), freeing
-- the remote connection's PGconn as changing_xact_state is true
select * from ft1;
ERROR: connection to server "loopback" was lost
rollback to s1;
-- This calls close_cursor() on the PGconn cached in PgFdwScanState,
-- which was freed above, leading to a server crash
close c1;

I think the root cause is that it is too early to free the PGconn in
pgfdw_reject_incomplete_xact_state_change() even if the connection is
in a state where we cannot use it any further; I think we should delay
that until abort cleanup (ie, pgfdw_xact_callback()). Attached is a
patch for that.

Best regards,
Etsuro Fujita
<fix-connection-handling-in-postgres-fdw.patch>

Hi Etsuro-san,

I can reproduce the server crash following your procedure, and I traced the problem.

The issue is that, during select * from ft1, pgfdw_reject_incomplete_xact_state_change() calls disconnect_pg_server(), which destroys conn and sets ConnCacheEntry->conn = NULL, but does not update PgFdwScanState->conn. As a result, when "close c1" is executed later, PgFdwScanState->conn points to stale memory with random contents.

I am not sure we should still allow further commands to run after select * from ft1, given that it has already raised: "ERROR: connection to server "loopback" was lost”. Maybe we should not keep going as if the connection were still there.

I am not very familiar with the FDW code, so I am not ready to suggest a concrete fix. But it seems wrong to let later paths keep using PgFdwScanState->conn after select * from ft1 has already failed with connection loss. My guess is that we either need to invalidate all dependent state when disconnect_pg_server() runs, or otherwise prevent later cleanup paths from touching the cached PGconn *.

Best regards,
--
Chao Li (Evan)
HighGo Software Co., Ltd.
https://www.highgo.com/

Hi,

On 19/03/26 11:56, Etsuro Fujita wrote:

I think the root cause is that it is too early to free the PGconn in
pgfdw_reject_incomplete_xact_state_change() even if the connection is
in a state where we cannot use it any further; I think we should delay
that until abort cleanup (ie, pgfdw_xact_callback()). Attached is a
patch for that.

IIUC, with this patch, disconnect_pg_server() will be called at
pgfdw_xact_callback() via pgfdw_reset_xact_state() when the top level
transaction is rollback right?

I've tested and it seems to fix the issue, when "close c1;" is
executed "conn" points to a valid connection pointer and this
connection is properly disconnected when the top level transactions is
rollback.

This issue is reproducible on v14, so I think that we need a back-port.

--
Matheus Alcantara
EDB: https://www.enterprisedb.com

On 20/03/26 05:27, Chao Li wrote:

I can reproduce the server crash following your procedure, and I traced the problem.

The issue is that, during select * from ft1, pgfdw_reject_incomplete_xact_state_change() calls disconnect_pg_server(), which destroys conn and sets ConnCacheEntry->conn = NULL, but does not update PgFdwScanState->conn. As a result, when "close c1" is executed later, PgFdwScanState->conn points to stale memory with random contents.

I am not sure we should still allow further commands to run after select * from ft1, given that it has already raised: "ERROR: connection to server "loopback" was lost”. Maybe we should not keep going as if the connection were still there.

I am not very familiar with the FDW code, so I am not ready to suggest a concrete fix. But it seems wrong to let later paths keep using PgFdwScanState->conn after select * from ft1 has already failed with connection loss. My guess is that we either need to invalidate all dependent state when disconnect_pg_server() runs, or otherwise prevent later cleanup paths from touching the cached PGconn *.

Although I agree with this I think that it will be a quite invasive
change to fix this issue, considering that it should be back-ported.

I see two ways to implement this:

1. ForeignScanState->conn points to a ConnCacheEntry and it access the
PGConn via ConnCacheEntry->conn, so it can check if != NULL before using.

2. Make pgfdw_reject_incomplete_xact_state_change() or
disconnect_pg_server() receive a PgFdwConnState and add a new field on
this state to represent that the connection is closed and them check
this field on the proper code paths.

With the patch proposed on the previous email the server don't crash
and any use of PgFdwScanState->conn will make the command to fail with
something like this:

ERROR: 08006: no connection to the server
CONTEXT: remote SQL command: CLOSE c1
LOCATION: pgfdw_report_internal, connection.c:1059

So I don't think that this change would be worth, or I'm missing
something? What do you think?

--
Matheus Alcantara
EDB: https://www.enterprisedb.com

Hi Chao,

On Fri, Mar 20, 2026 at 5:27 PM Chao Li <li.evan.chao@gmail.com> wrote:

I can reproduce the server crash following your procedure, and I traced the problem.

The issue is that, during select * from ft1, pgfdw_reject_incomplete_xact_state_change() calls disconnect_pg_server(), which destroys conn and sets ConnCacheEntry->conn = NULL, but does not update PgFdwScanState->conn. As a result, when "close c1" is executed later, PgFdwScanState->conn points to stale memory with random contents.

That is right.

I am not sure we should still allow further commands to run after select * from ft1, given that it has already raised: "ERROR: connection to server "loopback" was lost”. Maybe we should not keep going as if the connection were still there.

The ERROR is thrown within a subtransaction, not within the main
transaction, so we *should* allow commands (that don't use the
connection) to run *after* rolling back to the subtransaction.

(A transaction, like the example one, whose subtransactions failed
abort cleanup due to a connection issue needs to abort in the end, as
it can't commit the remote transaction anymore. Considering this
limitation, the above behavior might be surprising to users. We might
need to do something about this, but I think that that is another
issue and thus should be discussed separately.)

I am not very familiar with the FDW code, so I am not ready to suggest a concrete fix. But it seems wrong to let later paths keep using PgFdwScanState->conn after select * from ft1 has already failed with connection loss.

Why? While we can't use the connection any further, with the patch we
can still use the cashed PGconn, as pointed out by Matheus downthread.

My guess is that we either need to invalidate all dependent state when disconnect_pg_server() runs, or otherwise prevent later cleanup paths from touching the cached PGconn *.

I don't think that that is a good idea, because that 1) would require
invasive changes to the existing code, which isn't good especially for
back-patching, as also mentioned by Matheus downthread, and 2) would
result in something that is essentially the same as what I proposed;
in other words, that would be a lot of effort with little result.

Thanks for looking at this!

Best regards,
Etsuro Fujita

Hi Matheus,

On Sat, Mar 21, 2026 at 1:00 AM Matheus Alcantara
<matheusssilv97@gmail.com> wrote:

On 19/03/26 11:56, Etsuro Fujita wrote:

I think the root cause is that it is too early to free the PGconn in
pgfdw_reject_incomplete_xact_state_change() even if the connection is
in a state where we cannot use it any further; I think we should delay
that until abort cleanup (ie, pgfdw_xact_callback()). Attached is a
patch for that.

IIUC, with this patch, disconnect_pg_server() will be called at
pgfdw_xact_callback() via pgfdw_reset_xact_state() when the top level
transaction is rollback right?

That is right.

I've tested and it seems to fix the issue, when "close c1;" is
executed "conn" points to a valid connection pointer and this
connection is properly disconnected when the top level transactions is
rollback.

This issue is reproducible on v14, so I think that we need a back-port.

Yes, I am thinking of back-patching the fix to all supported versions,
if no objections.

Thanks for testing/reviewing the patch!

Best regards,
Etsuro Fujita

On Sat, Mar 21, 2026 at 1:14 AM Matheus Alcantara
<matheusssilv97@gmail.com> wrote:

On 20/03/26 05:27, Chao Li wrote:

I am not very familiar with the FDW code, so I am not ready to suggest a concrete fix. But it seems wrong to let later paths keep using PgFdwScanState->conn after select * from ft1 has already failed with connection loss. My guess is that we either need to invalidate all dependent state when disconnect_pg_server() runs, or otherwise prevent later cleanup paths from touching the cached PGconn *.

Although I agree with this I think that it will be a quite invasive
change to fix this issue, considering that it should be back-ported.

I see two ways to implement this:

1. ForeignScanState->conn points to a ConnCacheEntry and it access the
PGConn via ConnCacheEntry->conn, so it can check if != NULL before using.

2. Make pgfdw_reject_incomplete_xact_state_change() or
disconnect_pg_server() receive a PgFdwConnState and add a new field on
this state to represent that the connection is closed and them check
this field on the proper code paths.

With the patch proposed on the previous email the server don't crash
and any use of PgFdwScanState->conn will make the command to fail with
something like this:

ERROR: 08006: no connection to the server
CONTEXT: remote SQL command: CLOSE c1
LOCATION: pgfdw_report_internal, connection.c:1059

So I don't think that this change would be worth, or I'm missing
something? What do you think?

I don't feel the need to do these, either, for the reason mentioned upthread.

Thanks again!

Best regards,
Etsuro Fujita

On Mar 21, 2026, at 19:40, Etsuro Fujita <etsuro.fujita@gmail.com> wrote:

Hi Chao,

On Fri, Mar 20, 2026 at 5:27 PM Chao Li <li.evan.chao@gmail.com> wrote:

I can reproduce the server crash following your procedure, and I traced the problem.

The issue is that, during select * from ft1, pgfdw_reject_incomplete_xact_state_change() calls disconnect_pg_server(), which destroys conn and sets ConnCacheEntry->conn = NULL, but does not update PgFdwScanState->conn. As a result, when "close c1" is executed later, PgFdwScanState->conn points to stale memory with random contents.

That is right.

I am not sure we should still allow further commands to run after select * from ft1, given that it has already raised: "ERROR: connection to server "loopback" was lost”. Maybe we should not keep going as if the connection were still there.

The ERROR is thrown within a subtransaction, not within the main
transaction, so we *should* allow commands (that don't use the
connection) to run *after* rolling back to the subtransaction.

(A transaction, like the example one, whose subtransactions failed
abort cleanup due to a connection issue needs to abort in the end, as
it can't commit the remote transaction anymore. Considering this
limitation, the above behavior might be surprising to users. We might
need to do something about this, but I think that that is another
issue and thus should be discussed separately.)

Thanks for the explanation. Makes sense to me.

I am not very familiar with the FDW code, so I am not ready to suggest a concrete fix. But it seems wrong to let later paths keep using PgFdwScanState->conn after select * from ft1 has already failed with connection loss.

Why? While we can't use the connection any further, with the patch we
can still use the cashed PGconn, as pointed out by Matheus downthread.

My guess is that we either need to invalidate all dependent state when disconnect_pg_server() runs, or otherwise prevent later cleanup paths from touching the cached PGconn *.

I don't think that that is a good idea, because that 1) would require
invasive changes to the existing code, which isn't good especially for
back-patching, as also mentioned by Matheus downthread, and 2) would
result in something that is essentially the same as what I proposed;
in other words, that would be a lot of effort with little result.

Makes sense. Then the patch looks good to me.

Best regards,
--
Chao Li (Evan)
HighGo Software Co., Ltd.
https://www.highgo.com/

On Thu, Mar 19, 2026 at 11:56:13PM +0900, Etsuro Fujita wrote:

I got an offline report from my colleague Zhibai Song that
close_cursor() is called for a freed PGconn, leading to a server
crash. Here is a reproducer (the original reproducer he provided is a
bit complex, so I simplified it):

Hmm. We have one isolation test with a pg_terminate_backend() that
checks for a backend terminating, see temp-schema-cleanup.

Do you think that it would be worth having a test for this scenario,
where we could terminate a remote connection? An isolation test may
work, I hope, providing some insurance with the order of the
operations able to trigger the use-after-free behavior.

I think the root cause is that it is too early to free the PGconn in
pgfdw_reject_incomplete_xact_state_change() even if the connection is
in a state where we cannot use it any further; I think we should delay
that until abort cleanup (ie, pgfdw_xact_callback()). Attached is a
patch for that.

Removing the early disconnect() means that we should try to cover our
tracks in all the existing ,code paths where we finish the
top-transaction. Looking at the locations of
pgfdw_reset_xact_state(), it looks like you are getting yourself
covered.
--
Michael