Fix privilege checks for pg_prewarm() on indexes.

 pg_prewarm() currently checks for SELECT privileges on the target
 relation.  However, indexes do not have access rights of their own,
 so a role may be denied permission to prewarm an index despite
 having the SELECT privilege on its parent table.  This commit fixes
 this by locking the parent table before the index (to avoid
 deadlocks) and checking for SELECT on the parent table.  Note that
 the code is largely borrowed from
 amcheck_lock_relation_and_check().
 
 An obvious downside of this change is the extra AccessShareLock on
 the parent table during prewarming, but that isn't expected to
 cause too much trouble in practice.
 
 Author: Ayush Vatsa <ayushvatsa1810@gmail.com>
 Co-authored-by: Nathan Bossart <nathandbossart@gmail.com>
 Reviewed-by: Tom Lane <tgl@sss.pgh.pa.us>
 Reviewed-by: Jeff Davis <pgsql@j-davis.com>
 Discussion: https://postgr.es/m/CACX%2BKaMz2ZoOojh0nQ6QNBYx8Ak1Dkoko%3DD4FSb80BYW%2Bo8CHQ%40mail.gmail.com
 Backpatch-through: 13
 
 

diff --git a/contrib/pg_prewarm/pg_prewarm.c b/contrib/pg_prewarm/pg_prewarm.c
index b968933ea8b6c8c98287b3f6c9bb62415d282069..5b519a2c854229328e6a76713cae5e5677f18baf 100644 (file)
--- a/contrib/pg_prewarm/pg_prewarm.c
+++ b/contrib/pg_prewarm/pg_prewarm.c
@@ -16,9 +16,11 @@
 #include <unistd.h>
 
 #include "access/relation.h"
+#include "catalog/index.h"
 #include "fmgr.h"
 #include "miscadmin.h"
 #include "storage/bufmgr.h"
+#include "storage/lmgr.h"
 #include "storage/read_stream.h"
 #include "storage/smgr.h"
 #include "utils/acl.h"
@@ -71,6 +73,8 @@ pg_prewarm(PG_FUNCTION_ARGS)
    char       *ttype;
    PrewarmType ptype;
    AclResult   aclresult;
+   char        relkind;
+   Oid         privOid;
 
    /* Basic sanity checking. */
    if (PG_ARGISNULL(0))
@@ -106,9 +110,43 @@ pg_prewarm(PG_FUNCTION_ARGS)
    forkString = text_to_cstring(forkName);
    forkNumber = forkname_to_number(forkString);
 
-   /* Open relation and check privileges. */
+   /*
+    * Open relation and check privileges.  If the relation is an index, we
+    * must check the privileges on its parent table instead.
+    */
+   relkind = get_rel_relkind(relOid);
+   if (relkind == RELKIND_INDEX ||
+       relkind == RELKIND_PARTITIONED_INDEX)
+   {
+       privOid = IndexGetRelation(relOid, true);
+
+       /* Lock table before index to avoid deadlock. */
+       if (OidIsValid(privOid))
+           LockRelationOid(privOid, AccessShareLock);
+   }
+   else
+       privOid = relOid;
+
    rel = relation_open(relOid, AccessShareLock);
-   aclresult = pg_class_aclcheck(relOid, GetUserId(), ACL_SELECT);
+
+   /*
+    * It's possible that the relation with OID "privOid" was dropped and the
+    * OID was reused before we locked it.  If that happens, we could be left
+    * with the wrong parent table OID, in which case we must ERROR.  It's
+    * possible that such a race would change the outcome of
+    * get_rel_relkind(), too, but the worst case scenario there is that we'll
+    * check privileges on the index instead of its parent table, which isn't
+    * too terrible.
+    */
+   if (!OidIsValid(privOid) ||
+       (privOid != relOid &&
+        privOid != IndexGetRelation(relOid, true)))
+       ereport(ERROR,
+               (errcode(ERRCODE_UNDEFINED_TABLE),
+                errmsg("could not find parent table of index \"%s\"",
+                       RelationGetRelationName(rel))));
+
+   aclresult = pg_class_aclcheck(privOid, GetUserId(), ACL_SELECT);
    if (aclresult != ACLCHECK_OK)
        aclcheck_error(aclresult, get_relkind_objtype(rel->rd_rel->relkind), get_rel_name(relOid));
 
@@ -233,8 +271,11 @@ pg_prewarm(PG_FUNCTION_ARGS)
        read_stream_end(stream);
    }
 
-   /* Close relation, release lock. */
+   /* Close relation, release locks. */
    relation_close(rel, AccessShareLock);
 
+   if (privOid != relOid)
+       UnlockRelationOid(privOid, AccessShareLock);
+
    PG_RETURN_INT64(blocks_done);
 }

diff --git a/contrib/pg_prewarm/t/001_basic.pl b/contrib/pg_prewarm/t/001_basic.pl
index 0a8259d367854dd4c32998f4f96b899401784c68..a77ab67d29e83a80291cbe9579681e9ccbd02d35 100644 (file)
--- a/contrib/pg_prewarm/t/001_basic.pl
+++ b/contrib/pg_prewarm/t/001_basic.pl
@@ -23,7 +23,9 @@ $node->start;
 $node->safe_psql("postgres",
        "CREATE EXTENSION pg_prewarm;\n"
      . "CREATE TABLE test(c1 int);\n"
-     . "INSERT INTO test SELECT generate_series(1, 100);");
+     . "INSERT INTO test SELECT generate_series(1, 100);\n"
+     . "CREATE INDEX test_idx ON test(c1);\n"
+     . "CREATE ROLE test_user LOGIN;");
 
 # test read mode
 my $result =
@@ -42,6 +44,31 @@ ok( (        $stdout =~ qr/^[1-9][0-9]*$/
          or $stderr =~ qr/prefetch is not supported by this build/),
    'prefetch mode succeeded');
 
+# test_user should be unable to prewarm table/index without privileges
+($cmdret, $stdout, $stderr) =
+  $node->psql(
+    "postgres", "SELECT pg_prewarm('test');",
+    extra_params => [ '--username' => 'test_user' ]);
+ok($stderr =~ /permission denied for table test/, 'pg_prewarm failed as expected');
+($cmdret, $stdout, $stderr) =
+  $node->psql(
+    "postgres", "SELECT pg_prewarm('test_idx');",
+    extra_params => [ '--username' => 'test_user' ]);
+ok($stderr =~ /permission denied for index test_idx/, 'pg_prewarm failed as expected');
+
+# test_user should be able to prewarm table/index with privileges
+$node->safe_psql("postgres", "GRANT SELECT ON test TO test_user;");
+$result =
+  $node->safe_psql(
+    "postgres", "SELECT pg_prewarm('test');",
+    extra_params => [ '--username' => 'test_user' ]);
+like($result, qr/^[1-9][0-9]*$/, 'pg_prewarm succeeded as expected');
+$result =
+  $node->safe_psql(
+    "postgres", "SELECT pg_prewarm('test_idx');",
+    extra_params => [ '--username' => 'test_user' ]);
+like($result, qr/^[1-9][0-9]*$/, 'pg_prewarm succeeded as expected');
+
 # test autoprewarm_dump_now()
 $result = $node->safe_psql("postgres", "SELECT autoprewarm_dump_now();");
 like($result, qr/^[1-9][0-9]*$/, 'autoprewarm_dump_now succeeded');