Commit
b7b0f3f2724 ("Use streaming I/O in sequential scans") routed
sequential scans through read_stream_next_buffer(), bypassing the
RELATION_IS_OTHER_TEMP() check in ReadBufferExtended(). As a result,
a superuser can attempt to read or modify temp tables of other
sessions through the read-stream path. When the query plan uses no index,
SELECT/UPDATE/DELETE/MERGE silently see no rows / report zero affected rows,
and COPY produces an empty output -- because the buffer manager has no
visibility into the owning session's local buffers and silently returns
nothing. Any query plan that uses, for instance, a btree index
still errors out via the existing check in ReadBufferExtended(), which
is reached from hio.c and nbtree respectively, but this is incidental.
Fix by enforcing RELATION_IS_OTHER_TEMP() at the three additional
buffer-manager entry points:
- read_stream_begin_impl() rejects the read at stream setup time,
covering sequential and bitmap scans that go through the
read-stream path.
- ReadBuffer_common() becomes the canonical place for the check,
consolidating the existing one previously kept in
ReadBufferExtended(). All ReadBufferExtended() callers go through
ReadBuffer_common(), so the consolidation is behavior-preserving.
- StartReadBuffersImpl() catches direct callers of StartReadBuffers()
that bypass both of the above. This is currently defense-in-depth,
but documents the contract for future code.
The companion test in src/test/modules/test_misc was added in the
preceding commit; this commit updates the assertions for SELECT,
UPDATE, DELETE, MERGE, and COPY (which previously documented the
bug as silent success) to expect the new error.
Author: Jim Jones <jim.jones@uni-muenster.de>
Author: Daniil Davydov <3danissimo@gmail.com>
Co-authored-by: Alexander Korotkov <aekorotkov@gmail.com>
Reviewed-by: Michael Paquier <michael@paquier.xyz>
Reviewed-by: Soumya S Murali <soumyamurali.work@gmail.com>
Reviewed-by: Tom Lane <tgl@sss.pgh.pa.us>
Discussion: https://postgr.es/m/CAJDiXghdFcZ8%3Dnh4G69te7iRr3Q0uFyXxb3ZdG09_GTNZXwH0g%40mail.gmail.com
Backpatch-through: 17
uint32 max_possible_buffer_limit;
Oid tablespace_id;
+ /*
+ * 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 (rel && RELATION_IS_OTHER_TEMP(rel))
+ ereport(ERROR,
+ (errcode(ERRCODE_FEATURE_NOT_SUPPORTED),
+ errmsg("cannot access temporary tables of other sessions")));
+
/*
* Decide how many I/Os we will allow to run at the same time. This
* number also affects how far we look ahead for opportunities to start
if (RelationUsesLocalBuffers(reln))
{
- /* see comments in ReadBufferExtended */
+ /* see comments in ReadBuffer_common */
if (RELATION_IS_OTHER_TEMP(reln))
ereport(ERROR,
(errcode(ERRCODE_FEATURE_NOT_SUPPORTED),
{
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.
+ * miss. The other-session temp-relation check is enforced by
+ * ReadBuffer_common().
*/
buf = ReadBuffer_common(reln, RelationGetSmgr(reln), 0,
forkNum, blockNum, mode, strategy);
int flags;
char persistence;
+ /*
+ * 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. This is the canonical place for the check,
+ * covering the ReadBufferExtended() entry point and any other caller that
+ * supplies a Relation.
+ */
+ if (rel && RELATION_IS_OTHER_TEMP(rel))
+ ereport(ERROR,
+ (errcode(ERRCODE_FEATURE_NOT_SUPPORTED),
+ errmsg("cannot access temporary tables of other sessions")));
+
/*
* Backward compatibility path, most code should use ExtendBufferedRel()
* instead, as acquiring the extension lock inside ExtendBufferedRel()
Assert(*nblocks > 0);
Assert(*nblocks <= MAX_IO_COMBINE_LIMIT);
+ /* see comments in ReadBuffer_common */
+ if (operation->rel && RELATION_IS_OTHER_TEMP(operation->rel))
+ ereport(ERROR,
+ (errcode(ERRCODE_FEATURE_NOT_SUPPORTED),
+ errmsg("cannot access temporary tables of other sessions")));
+
if (operation->persistence == RELPERSISTENCE_TEMP)
{
io_context = IOCONTEXT_NORMAL;
*
* Reading another session's temp-table data through never works right:
* the owning session keeps the data in its private local buffer pool,
- * which we cannot access. The macro is therefore used at the buffer-manager
- * level to reject such accesses, and by command-level code (TRUNCATE,
- * ALTER TABLE, VACUUM, CLUSTER, REINDEX, ...) for command-specific error
- * messages.
- *
- * Currenlty buffer manager checks include only ReadBufferExtended(), and
- * PrefetchBuffer(); while ReadBuffer_common(), read_stream_begin_impl(), and
- * StartReadBuffersImpl() are not covered. As a result, read paths that
- * bypass ReadBufferExtended() -- notably sequential scans that go through
- * the read-stream API -- silently return no rows when targeted at another
- * session's temp table instead of failing.
+ * which we cannot access. Existing buffer-manager entry points
+ * (ReadBuffer_common(), StartReadBuffersImpl(), read_stream_begin_impl(),
+ * and PrefetchBuffer()) already enforce this; any new buffer-access entry
+ * points must do the same. Command-level code (TRUNCATE, ALTER TABLE,
+ * VACUUM, CLUSTER, REINDEX, ...) additionally uses this macro for
+ * command-specific error messages.
*
* Beware of multiple eval of argument
*/
# DML and SELECT have to read the table's data and therefore go through
# the buffer manager. With no index on the table, the planner cannot
# use index access, so SELECT/UPDATE/DELETE/MERGE/COPY all run through
-# the read-stream path.
-#
-# XXX: in current code, the read-stream path bypasses the
-# RELATION_IS_OTHER_TEMP() check, so these commands silently see no
-# rows / report zero affected rows -- the visible symptom of the bug
-# this test suite documents. A follow-up patch will route the check
-# through read_stream_begin_impl() and these assertions will be
-# updated to expect "cannot access temporary tables of other sessions".
+# the read-stream path and are caught by read_stream_begin_impl().
$node->psql(
'postgres',
"SELECT val FROM $tempschema.foo;",
- stdout => \$stdout,
stderr => \$stderr);
-is($stderr, '', 'SELECT (currently no error -- bug to be fixed)');
+like(
+ $stderr,
+ qr/cannot access temporary tables of other sessions/,
+ 'SELECT (seqscan via read_stream)');
# INSERT goes through hio.c which calls ReadBufferExtended() to find a
-# page with free space; that hits the existing check before any data is
-# written. This case currently errors as expected.
+# page with free space; that hits the existing check before any data
+# is written.
$node->psql(
'postgres',
"INSERT INTO $tempschema.foo VALUES (73);",
'postgres',
"UPDATE $tempschema.foo SET val = NULL;",
stderr => \$stderr);
-is($stderr, '', 'UPDATE (currently no error -- bug to be fixed)');
+like($stderr, qr/cannot access temporary tables of other sessions/, 'UPDATE');
$node->psql('postgres', "DELETE FROM $tempschema.foo;", stderr => \$stderr);
-is($stderr, '', 'DELETE (currently no error -- bug to be fixed)');
+like($stderr, qr/cannot access temporary tables of other sessions/, 'DELETE');
$node->psql(
'postgres',
"MERGE INTO $tempschema.foo USING (VALUES (42)) AS s(val) "
. "ON foo.val = s.val WHEN MATCHED THEN DELETE;",
stderr => \$stderr);
-is($stderr, '', 'MERGE (currently no error -- bug to be fixed)');
+like($stderr, qr/cannot access temporary tables of other sessions/, 'MERGE');
$node->psql('postgres', "COPY $tempschema.foo TO STDOUT;",
stderr => \$stderr);
-is($stderr, '', 'COPY (currently no error -- bug to be fixed)');
+like($stderr, qr/cannot access temporary tables of other sessions/, 'COPY');
# DDL and maintenance commands have their own command-specific checks
# (older than the buffer-manager check above), so they fail with
projects / thirdparty / postgresql.git / commitdiff
