Обсуждение: We broke the defense against accessing other sessions' temp tables
In session 1:
regression=# create temp table tt(f1 int);
CREATE TABLE
regression=# insert into tt values(44);
INSERT 0 1
regression=# \d tt
Table "pg_temp_77.tt"
Column | Type | Collation | Nullable | Default
--------+---------+-----------+----------+---------
f1 | integer | | |
In session 2:
regression=# select * from pg_temp_77.tt; -- use the schema shown in session 1
f1
----
(0 rows)
Branches before 17 show the expected failure
ERROR: cannot access temporary tables of other sessions
but that's gone missing :-(. Apparently, we refactored things enough
that ReadBufferExtended is not used for a simple seqscan, and since
that's where the check for an other-session temp table is, we get
no error and instead bogus results. I did not bother to bisect to
find a culprit commit, but given that the failure manifests in 17,
I'm betting the stream I/O stuff is at fault.
I experimented with moving the check into PinBufferForBlock, and
that seems to work correctly, but I wonder if someone who's messed
with this code more recently than I would prefer a different
solution.
regards, tom lane
diff --git a/src/backend/storage/buffer/bufmgr.c b/src/backend/storage/buffer/bufmgr.c
index fe470de63f2..e27d74a24c2 100644
--- a/src/backend/storage/buffer/bufmgr.c
+++ b/src/backend/storage/buffer/bufmgr.c
@@ -655,7 +655,7 @@ PrefetchBuffer(Relation reln, ForkNumber forkNum, BlockNumber blockNum)
if (RelationUsesLocalBuffers(reln))
{
- /* see comments in ReadBufferExtended */
+ /* see comments in PinBufferForBlock */
if (RELATION_IS_OTHER_TEMP(reln))
ereport(ERROR,
(errcode(ERRCODE_FEATURE_NOT_SUPPORTED),
@@ -807,16 +807,6 @@ ReadBufferExtended(Relation reln, ForkNumber forkNum, BlockNumber blockNum,
{
Buffer buf;
- /*
- * Reject attempts to read non-local temporary relations; we would be
- * likely to get wrong data since we have no visibility into the owning
- * session's local buffers.
- */
- if (RELATION_IS_OTHER_TEMP(reln))
- ereport(ERROR,
- (errcode(ERRCODE_FEATURE_NOT_SUPPORTED),
- errmsg("cannot access temporary tables of other sessions")));
-
/*
* Read the buffer, and update pgstat counters to reflect a cache hit or
* miss.
@@ -1128,6 +1118,18 @@ PinBufferForBlock(Relation rel,
if (persistence == RELPERSISTENCE_TEMP)
{
+ /*
+ * Reject attempts to read non-local temporary relations; we would be
+ * likely to get wrong data since we have no visibility into the
+ * owning session's local buffers. We don't expect to get here
+ * without a relcache entry (see ReadBufferWithoutRelcache).
+ */
+ Assert(rel);
+ if (RELATION_IS_OTHER_TEMP(rel))
+ ereport(ERROR,
+ (errcode(ERRCODE_FEATURE_NOT_SUPPORTED),
+ errmsg("cannot access temporary tables of other sessions")));
+
io_context = IOCONTEXT_NORMAL;
io_object = IOOBJECT_TEMP_RELATION;
}
On Sun, Sep 21, 2025 at 01:32:59PM -0400, Tom Lane wrote: > but that's gone missing :-(. Apparently, we refactored things enough > that ReadBufferExtended is not used for a simple seqscan, and since > that's where the check for an other-session temp table is, we get > no error and instead bogus results. I did not bother to bisect to > find a culprit commit, but given that the failure manifests in 17, > I'm betting the stream I/O stuff is at fault. > > I experimented with moving the check into PinBufferForBlock, and > that seems to work correctly, but I wonder if someone who's messed > with this code more recently than I would prefer a different > solution. Given that this problem exists in PG 17, I assume this will not affect the PG 18 release this week. -- Bruce Momjian <bruce@momjian.us> https://momjian.us EDB https://enterprisedb.com Do not let urgent matters crowd out time for investment in the future.
Bruce Momjian <bruce@momjian.us> writes:
> Given that this problem exists in PG 17, I assume this will not affect
> the PG 18 release this week.
Yeah, if it were 18-only we'd need a powwow about whether to break
release freeze. But since it's been there since 17 and nobody
noticed, I'm content to wait till after 18.0 to fix it.
I believe you need to be superuser to reach the bug at all, otherwise
permissions checks should stop you earlier. This makes it less urgent
than it would otherwise be.
regards, tom lane
On Sun, Sep 21, 2025 at 01:44:11PM -0400, Tom Lane wrote: > Bruce Momjian <bruce@momjian.us> writes: > > Given that this problem exists in PG 17, I assume this will not affect > > the PG 18 release this week. > > Yeah, if it were 18-only we'd need a powwow about whether to break > release freeze. But since it's been there since 17 and nobody > noticed, I'm content to wait till after 18.0 to fix it. > > I believe you need to be superuser to reach the bug at all, otherwise > permissions checks should stop you earlier. This makes it less urgent > than it would otherwise be. Oh, good to know. -- Bruce Momjian <bruce@momjian.us> https://momjian.us EDB https://enterprisedb.com Do not let urgent matters crowd out time for investment in the future.
On Sun, 21 Sept 2025 at 19:44, Tom Lane <tgl@sss.pgh.pa.us> wrote: > Yeah, if it were 18-only we'd need a powwow about whether to break > release freeze. But since it's been there since 17 and nobody > noticed, I'm content to wait till after 18.0 to fix it. People definitely noticed[1], but I agree that there's no reason to rush this fix out before the 18 release. Patch looks good, but obviously needs some tests. [1]: https://www.postgresql.org/message-id/flat/CAMEv5_syU0ZopE-2Wr8A8QksqrCyYT2hW06Rgw4RSPdyJO-%3Dfw%40mail.gmail.com
On Sun, Sep 21, 2025 at 12:19 PM Jelte Fennema-Nio <postgres@jeltef.nl> wrote:
On Sun, 21 Sept 2025 at 19:44, Tom Lane <tgl@sss.pgh.pa.us> wrote:
> Yeah, if it were 18-only we'd need a powwow about whether to break
> release freeze. But since it's been there since 17 and nobody
> noticed, I'm content to wait till after 18.0 to fix it.
People definitely noticed[1], but I agree that there's no reason to
rush this fix out before the 18 release.
Patch looks good, but obviously needs some tests.
[1]: https://www.postgresql.org/message-id/flat/CAMEv5_syU0ZopE-2Wr8A8QksqrCyYT2hW06Rgw4RSPdyJO-%3Dfw%40mail.gmail.com
I'm presuming this report is also about the same underlying issue.
It too has a patch included.
David J.
Hi
On 9/21/25 21:39, David G. Johnston wrote:
> On Sun, Sep 21, 2025 at 12:19 PM Jelte Fennema-Nio <postgres@jeltef.nl
> <mailto:postgres@jeltef.nl>> wrote:
>
> On Sun, 21 Sept 2025 at 19:44, Tom Lane <tgl@sss.pgh.pa.us
> <mailto:tgl@sss.pgh.pa.us>> wrote:
> > Yeah, if it were 18-only we'd need a powwow about whether to break
> > release freeze. But since it's been there since 17 and nobody
> > noticed, I'm content to wait till after 18.0 to fix it.
>
> People definitely noticed[1], but I agree that there's no reason to
> rush this fix out before the 18 release.
>
> Patch looks good, but obviously needs some tests.
Here a few tests:
== session 1 ==
$ /usr/local/postgres-dev/bin/psql postgres
psql (19devel)
Type "help" for help.
postgres=# CREATE TEMPORARY TABLE tmp AS SELECT 42 AS val;
SELECT 1
postgres=# \d tmp
Table "pg_temp_81.tmp"
Column | Type | Collation | Nullable | Default
--------+---------+-----------+----------+---------
val | integer | | |
== session 2 ==
$ /usr/local/postgres-dev/bin/psql postgres
psql (19devel)
Type "help" for help.
-- previously returned O rows
postgres=# SELECT * FROM pg_temp_81.tmp;
ERROR: cannot access temporary tables of other sessions
-- previously returned DELETE 0
postgres=# DELETE FROM pg_temp_81.tmp;
ERROR: cannot access temporary tables of other sessions
postgres=# TRUNCATE TABLE pg_temp_81.tmp;
ERROR: cannot truncate temporary tables of other sessions
-- previously returned UPDATE 0
postgres=# UPDATE pg_temp_81.tmp SET val = NULL;
ERROR: cannot access temporary tables of other sessions
postgres=# INSERT INTO pg_temp_81.tmp VALUES (0);
ERROR: cannot access temporary tables of other sessions
postgres=# COPY pg_temp_81.tmp TO '/tmp/x';
ERROR: cannot access temporary tables of other sessions
postgres=# ALTER TABLE pg_temp_81.tmp ADD COLUMN foo text;
ERROR: cannot alter temporary tables of other sessions
Is ALTER TABLE ... RENAME a loophole? I tested this in PostgreSQL 15.14
and the result was the same:
postgres=# ALTER TABLE pg_temp_81.tmp RENAME TO foo;
ALTER TABLE
== session 1 ==
postgres=# \d tmp
Did not find any relation named "tmp".
postgres=# \d foo
Table "pg_temp_81.foo"
Column | Type | Collation | Nullable | Default
--------+---------+-----------+----------+---------
val | integer | | |
I also noticed that it is possible to LOCK a temp table from another
session (as superuser).
== session 1 ==
$ /usr/local/postgres-dev/bin/psql postgres
psql (19devel)
Type "help" for help.
postgres=# CREATE TEMPORARY TABLE tmp AS SELECT 42 AS val;
SELECT 1
postgres=# \d tmp
Table "pg_temp_86.tmp"
Column | Type | Collation | Nullable | Default
--------+---------+-----------+----------+---------
val | integer | | |
== session 2 ==
$ /usr/local/postgres-dev/bin/psql postgres
psql (19devel)
Type "help" for help.
postgres=# BEGIN;
BEGIN
postgres=*# LOCK TABLE pg_temp_86.tmp IN ACCESS EXCLUSIVE MODE;
LOCK TABLE
postgres=*#
Doesn't it mean that one session can trivially DoS another session's
private temp table?
== session 1 ==
postgres=# SELECT locktype, relation::regclass, mode, granted, pid
FROM pg_locks
WHERE relation = 'pg_temp_86.tmp'::regclass::oid;
locktype | relation | mode | granted | pid
----------+----------+---------------------+---------+---------
relation | tmp | AccessExclusiveLock | t | 3286840
(1 row)
postgres=# SELECT * FROM tmp;
^CCancel request sent
ERROR: canceling statement due to user request
CONTEXT: waiting for AccessShareLock on relation 47632 of database 5
postgres=#
I didn't test all possible LOCK modes, but I suspect they all work.
== session 2 ==
postgres=# BEGIN;
BEGIN
postgres=*# LOCK TABLE pg_temp_86.tmp IN SHARE MODE;
LOCK TABLE
postgres=*#
== session 1 ==
postgres=# SELECT locktype, relation::regclass, mode, granted, pid
FROM pg_locks
WHERE relation = 'pg_temp_86.tmp'::regclass::oid;
locktype | relation | mode | granted | pid
----------+----------+-----------+---------+---------
relation | tmp | ShareLock | t | 3289767
(1 row)
postgres=# SELECT * FROM tmp;
val
-----
42
(1 row)
As expected with non-superusers it returns a permission denied for the
temp schema:
== session 2 ==
postgres=# CREATE USER u1;
CREATE ROLE
postgres=# SET ROLE u1;
SET
postgres=> BEGIN;
BEGIN
postgres=*> LOCK TABLE pg_temp_86.tmp IN ACCESS EXCLUSIVE MODE;
ERROR: permission denied for schema pg_temp_86
postgres=!>
Best regards, Jim
On 9/22/25 10:14, Jim Jones wrote:
> I also noticed that it is possible to LOCK a temp table from another
> session (as superuser).
It gets even stranger if the owner's session closes while there is a
LOCK in a different session:
== session 1 ==
$ /usr/local/postgres-dev/bin/psql postgres
psql (19devel)
Type "help" for help.
postgres=# SELECT pg_backend_pid();
pg_backend_pid
----------------
3520132
(1 row)
postgres=# CREATE TEMPORARY TABLE foo AS SELECT 42 AS val;
SELECT 1
postgres=# \d foo
Table "pg_temp_6.foo"
Column | Type | Collation | Nullable | Default
--------+---------+-----------+----------+---------
val | integer | | |
== session 2 ==
$ /usr/local/postgres-dev/bin/psql postgres
psql (19devel)
Type "help" for help.
postgres=# BEGIN;
BEGIN
postgres=*# LOCK TABLE pg_temp_6.foo IN ACCESS EXCLUSIVE MODE;
LOCK TABLE
postgres=*# SELECT pg_backend_pid();
pg_backend_pid
----------------
3520608
(1 row)
== session 1 ==
postgres=# SELECT locktype, relation::regclass, mode, granted, pid
FROM pg_locks
WHERE relation = 'pg_temp_6.foo'::regclass::oid;
locktype | relation | mode | granted | pid
----------+----------+---------------------+---------+---------
relation | foo | AccessExclusiveLock | t | 3520608
(1 row)
postgres=# \q
== session 3 ==
$ /usr/local/postgres-dev/bin/psql postgres
psql (19devel)
Type "help" for help.
postgres=# SELECT pg_backend_pid();
pg_backend_pid
----------------
3521711
(1 row)
postgres=# SELECT locktype, relation::regclass, mode, granted, pid
FROM pg_locks
WHERE relation = 'pg_temp_6.foo'::regclass::oid;
locktype | relation | mode | granted | pid
----------+---------------+---------------------+---------+---------
relation | pg_temp_6.foo | AccessExclusiveLock | t | 3520608
relation | pg_temp_6.foo | AccessExclusiveLock | f | 3520132
(2 rows)
== session 4 ==
$ /usr/local/postgres-dev/bin/psql postgres
psql (19devel)
Type "help" for help.
postgres=# SELECT pid, wait_event_type, wait_event, state
FROM pg_stat_activity
WHERE pid = 3520132;
pid | wait_event_type | wait_event | state
---------+-----------------+------------+-------
3520132 | Lock | relation | idle
(1 row)
pid 3520132 lives as long as session 2 holds a lock on pg_temp_6.foo.
Best, Jim
Jim Jones <jim.jones@uni-muenster.de> writes:
> I also noticed that it is possible to LOCK a temp table from another
> session (as superuser).
Sure. Superusers bypass all SQL permissions checks.
> Doesn't it mean that one session can trivially DoS another session's
> private temp table?
Superusers can trivially DoS anybody anytime: lock their tables,
corrupt their data, whatever. That's a feature not a bug.
regards, tom lane
On 10/4/25 19:38, Tom Lane wrote: > Sure. Superusers bypass all SQL permissions checks. True. I just didn't expect this behavior to extend to temporary tables as well. It was just a bit surprising that a temporary table can persist after its owning session ends, simply because another session is holding a lock on it --- without the owning session being aware of that. Best, Jim
Jim Jones <jim.jones@uni-muenster.de> writes:
> True. I just didn't expect this behavior to extend to temporary tables
> as well. It was just a bit surprising that a temporary table can persist
> after its owning session ends, simply because another session is holding
> a lock on it --- without the owning session being aware of that.
Actually the owning session is perfectly aware of that: it's waiting
on the lock to be released so it can finish dropping its temp tables.
The *client* likely isn't aware, because usually clients just close
the connection without waiting for end-of-session cleanup to happen.
regards, tom lane
On Mon, Sep 22, 2025 at 5:33 AM Tom Lane <tgl@sss.pgh.pa.us> wrote: > In session 1: > > regression=# create temp table tt(f1 int); > CREATE TABLE > regression=# insert into tt values(44); > INSERT 0 1 > regression=# \d tt > Table "pg_temp_77.tt" > Column | Type | Collation | Nullable | Default > --------+---------+-----------+----------+--------- > f1 | integer | | | > > In session 2: > > regression=# select * from pg_temp_77.tt; -- use the schema shown in session 1 > f1 > ---- > (0 rows) > > Branches before 17 show the expected failure > > ERROR: cannot access temporary tables of other sessions > > but that's gone missing :-(. Apparently, we refactored things enough > that ReadBufferExtended is not used for a simple seqscan, and since > that's where the check for an other-session temp table is, we get > no error and instead bogus results. I did not bother to bisect to > find a culprit commit, but given that the failure manifests in 17, > I'm betting the stream I/O stuff is at fault. That was me, and it looks like I managed to miss the earlier reports and analysis. Oops. > I experimented with moving the check into PinBufferForBlock, and > that seems to work correctly, but I wonder if someone who's messed > with this code more recently than I would prefer a different > solution. This looks logically correct at first glance, but I guess it would be nice to perform the check just once for a stream instead of once per block. Each stream only accesses one relation for its whole life*. That leads to the idea of leaving the check where it is in ReadBufferExtended() for non-stream code paths, but (1) duplicating it in read_stream_begin_relation(), (2) documenting in a comment above StartReadBuffersImpl() that it's the caller's responsibility not to try to read buffers in other backends' temporary tables, and (3) turning that proposed check in PinBufferForBlock() into an assertion just to catch programming errors. I can try that soon if you think that makes sense, unless someone else wants to. An alternative would be to perform the check somewhere in StartReadBuffers(), so that at least it's performed only once over sequential ranges up to io_combine_limit. I don't like that idea: I don't currently expect other users of StartReadBuffers() to sprout, so I'm not sure an ereport defence is warranted at that level, at the cost of extra code size and cycles in this path. We could always reconsider that if I turn out to be wrong in future development, but for now StartReadBuffers() is basically plumbing that allows a modularity border between buffer manager concerns and streaming concerns, and it seems OK to write down what the contract is and check it with an assertion, and then perform user-face ereport checks at the most efficient spot. Thinking bigger about the scope of the check, I wondered if it could make sense to block other backends' relations from being opened in the first place. Reading the older threads, I see that that was already being discussed[1] by Andres and Andrey (well they didn't say "open" but I guess that's what they meant?) in the context of cross-process DROP, which would presumably need a special I-know-what-I'm-doing flag to escape that check. Then I guess if we had the above-mentioned assertion, if a caller passed the escape hatch flag but doesn't in fact know what it's doing, at least that'd fail. Something to think about for master maybe. *FWIW, one of my prototypes for true AIO in recovery can switch between (smgr) relations in one stream, but there are no temporary relations in the picture in that context. [1] https://www.postgresql.org/message-id/flat/4xxau766dofbwugeyvjftra3g5f7ifaal2clgrbpr7jqotr4av%40d3ige2krpoza#3c0ef03be42942dcb7e4c305abb20ad7
Thomas Munro <thomas.munro@gmail.com> writes:
> On Mon, Sep 22, 2025 at 5:33 AM Tom Lane <tgl@sss.pgh.pa.us> wrote:
>> I experimented with moving the check into PinBufferForBlock, and
>> that seems to work correctly, but I wonder if someone who's messed
>> with this code more recently than I would prefer a different
>> solution.
> This looks logically correct at first glance, but I guess it would be
> nice to perform the check just once for a stream instead of once per
> block. Each stream only accesses one relation for its whole life*.
Fair point.
> That leads to the idea of leaving the check where it is in
> ReadBufferExtended() for non-stream code paths, but (1) duplicating it
> in read_stream_begin_relation(), (2) documenting in a comment above
> StartReadBuffersImpl() that it's the caller's responsibility not to
> try to read buffers in other backends' temporary tables, and (3)
> turning that proposed check in PinBufferForBlock() into an assertion
> just to catch programming errors. I can try that soon if you think
> that makes sense, unless someone else wants to.
If we put this check at a higher level, there's more risk of people
forgetting the check in new call paths (which is pretty much what
happened here). So I'm a bit leery of it. Still, a backup Assert
at the PinBufferForBlock level might be good enough.
Anyway, happy to turn this over to you, I'm just the messenger.
> Thinking bigger about the scope of the check, I wondered if it could
> make sense to block other backends' relations from being opened in the
> first place. Reading the older threads, I see that that was already
> being discussed[1] by Andres and Andrey (well they didn't say "open"
> but I guess that's what they meant?) in the context of cross-process
> DROP, which would presumably need a special I-know-what-I'm-doing flag
> to escape that check.
Yeah, I'm not sure that that's worth the complication. There is no
logical difficulty in dropping someone else's temp table, only in
trying to access its contents, so I think that a check at the buffer
manager level is really pretty much the right thing.
regards, tom lane
Hi Thomas On 10/5/25 03:55, Tom Lane wrote: > Anyway, happy to turn this over to you, I'm just the messenger. Just to avoid any duplication of effort: there's already a patch that substantially overlaps with this issue[1] Best regards, Jim 1 - https://commitfest.postgresql.org/patch/5379/
On Sun, Oct 5, 2025 at 11:02 PM Jim Jones <jim.jones@uni-muenster.de> wrote: > Just to avoid any duplication of effort: there's already a patch that > substantially overlaps with this issue[1] > 1 - https://commitfest.postgresql.org/patch/5379/ Oh, thanks Jim. Will take a look.