Fix a couple of misbehaviors rooted in the fact that the default creation

namespace isn't necessarily first in the search path (there could be implicit
schemas ahead of it).  Examples are

test=# set search_path TO s1;

test=# create view pg_timezone_names as select * from pg_timezone_names();
ERROR:  "pg_timezone_names" is already a view

test=# create table pg_class (f1 int primary key);
ERROR:  permission denied: "pg_class" is a system catalog

You'd expect these commands to create the requested objects in s1, since
names beginning with pg_ aren't supposed to be reserved anymore.  What is
happening is that we create the requested base table and then execute
additional commands (here, CREATE RULE or CREATE INDEX), and that code is
passed the same RangeVar that was in the original command.  Since that
RangeVar has schemaname = NULL, the secondary commands think they should do a
path search, and that means they find system catalogs that are implicitly in
front of s1 in the search path.

This is perilously close to being a security hole: if the secondary command
failed to apply a permission check then it'd be possible for unprivileged
users to make schema modifications to system catalogs.  But as far as I can
find, there is no code path in which a check doesn't occur.  Which makes it
just a weird corner-case bug for people who are silly enough to want to
name their tables the same as a system catalog.

The relevant code has changed quite a bit since 8.2, which means this patch
wouldn't work as-is in the back branches.  Since it's a corner case no one
has reported from the field, I'm not going to bother trying to back-patch.

diff --git a/src/backend/catalog/namespace.c b/src/backend/catalog/namespace.c
index 57c35300e168e34a70c08ec605483784d1a5fc28..057bbee086e5a94997cc4c7e813e85c9bf7830c4 100644 (file)
--- a/src/backend/catalog/namespace.c
+++ b/src/backend/catalog/namespace.c
@@ -228,7 +228,26 @@ RangeVarGetRelid(const RangeVar *relation, bool failOK)
                                                        relation->relname)));
        }
 
-       if (relation->schemaname)
+       /*
+        * If istemp is set, this is a reference to a temp relation.  The parser
+        * never generates such a RangeVar in simple DML, but it can happen in
+        * contexts such as "CREATE TEMP TABLE foo (f1 int PRIMARY KEY)".  Such a
+        * command will generate an added CREATE INDEX operation, which must be
+        * careful to find the temp table, even when pg_temp is not first in the
+        * search path.
+        */
+       if (relation->istemp)
+       {
+               if (relation->schemaname)
+                       ereport(ERROR,
+                                       (errcode(ERRCODE_INVALID_TABLE_DEFINITION),
+                                        errmsg("temporary tables cannot specify a schema name")));
+               if (OidIsValid(myTempNamespace))
+                       relId = get_relname_relid(relation->relname, myTempNamespace);
+               else                                    /* this probably can't happen? */
+                       relId = InvalidOid;
+       }
+       else if (relation->schemaname)
        {
                /* use exact schema given */
                namespaceId = LookupExplicitNamespace(relation->schemaname);

diff --git a/src/backend/commands/view.c b/src/backend/commands/view.c
index f7673a6273f9e93e0cb7f728e34722b0fb07d1a1..06d69a2a510b564ed0788080aec3ea6300fc2c0e 100644 (file)
--- a/src/backend/commands/view.c
+++ b/src/backend/commands/view.c
@@ -260,14 +260,14 @@ checkViewTupleDesc(TupleDesc newdesc, TupleDesc olddesc)
 }
 
 static void
-DefineViewRules(const RangeVar *view, Query *viewParse, bool replace)
+DefineViewRules(Oid viewOid, Query *viewParse, bool replace)
 {
        /*
         * Set up the ON SELECT rule.  Since the query has already been through
         * parse analysis, we use DefineQueryRewrite() directly.
         */
        DefineQueryRewrite(pstrdup(ViewSelectRuleName),
-                                          (RangeVar *) copyObject((RangeVar *) view),
+                                          viewOid,
                                           NULL,
                                           CMD_SELECT,
                                           true,
@@ -404,7 +404,9 @@ DefineView(ViewStmt *stmt, const char *queryString)
 
        /*
         * If the user didn't explicitly ask for a temporary view, check whether
-        * we need one implicitly.
+        * we need one implicitly.  We allow TEMP to be inserted automatically
+        * as long as the CREATE command is consistent with that --- no explicit
+        * schema name.
         */
        view = stmt->view;
        if (!view->istemp && isViewOnTempTable(viewParse))
@@ -441,7 +443,7 @@ DefineView(ViewStmt *stmt, const char *queryString)
        /*
         * Now create the rules associated with the view.
         */
-       DefineViewRules(view, viewParse, stmt->replace);
+       DefineViewRules(viewOid, viewParse, stmt->replace);
 }
 
 /*

diff --git a/src/backend/parser/parse_utilcmd.c b/src/backend/parser/parse_utilcmd.c
index 2e68e6a73be71014be642427bb06b3aeeff7a212..45583d97dc3c1c6eb75f70bc90aa8fa5c62bb203 100644 (file)
--- a/src/backend/parser/parse_utilcmd.c
+++ b/src/backend/parser/parse_utilcmd.c
@@ -143,6 +143,20 @@ transformCreateStmt(CreateStmt *stmt, const char *queryString)
         */
        stmt = (CreateStmt *) copyObject(stmt);
 
+       /*
+        * If the target relation name isn't schema-qualified, make it so.  This
+        * prevents some corner cases in which added-on rewritten commands might
+        * think they should apply to other relations that have the same name
+        * and are earlier in the search path.  "istemp" is equivalent to a
+        * specification of pg_temp, so no need for anything extra in that case.
+        */
+       if (stmt->relation->schemaname == NULL && !stmt->relation->istemp)
+       {
+               Oid             namespaceid = RangeVarGetCreationNamespace(stmt->relation);
+
+               stmt->relation->schemaname = get_namespace_name(namespaceid);
+       }
+
        /* Set up pstate */
        pstate = make_parsestate(NULL);
        pstate->p_sourcetext = queryString;

diff --git a/src/backend/rewrite/rewriteDefine.c b/src/backend/rewrite/rewriteDefine.c
index 3414a1b0c1e27dbcee3d4be63878588bf599bf00..5812cc4830a60272ca8193f7e6f4113ec4706c29 100644 (file)
--- a/src/backend/rewrite/rewriteDefine.c
+++ b/src/backend/rewrite/rewriteDefine.c
@@ -17,6 +17,7 @@
 #include "access/heapam.h"
 #include "catalog/dependency.h"
 #include "catalog/indexing.h"
+#include "catalog/namespace.h"
 #include "catalog/pg_rewrite.h"
 #include "miscadmin.h"
 #include "optimizer/clauses.h"
@@ -189,13 +190,17 @@ DefineRule(RuleStmt *stmt, const char *queryString)
 {
        List       *actions;
        Node       *whereClause;
+       Oid                     relId;
 
        /* Parse analysis ... */
        transformRuleStmt(stmt, queryString, &actions, &whereClause);
 
-       /* ... and execution */
+       /* ... find the relation ... */
+       relId = RangeVarGetRelid(stmt->relation, false);
+
+       /* ... and execute */
        DefineQueryRewrite(stmt->rulename,
-                                          stmt->relation,
+                                          relId,
                                           whereClause,
                                           stmt->event,
                                           stmt->instead,
@@ -213,7 +218,7 @@ DefineRule(RuleStmt *stmt, const char *queryString)
  */
 void
 DefineQueryRewrite(char *rulename,
-                                  RangeVar *event_obj,
+                                  Oid event_relid,
                                   Node *event_qual,
                                   CmdType event_type,
                                   bool is_instead,
@@ -221,7 +226,6 @@ DefineQueryRewrite(char *rulename,
                                   List *action)
 {
        Relation        event_relation;
-       Oid                     ev_relid;
        Oid                     ruleId;
        int                     event_attno;
        ListCell   *l;
@@ -235,13 +239,12 @@ DefineQueryRewrite(char *rulename,
         * grab ShareLock to lock out insert/update/delete actions.  But for now,
         * let's just grab AccessExclusiveLock all the time.
         */
-       event_relation = heap_openrv(event_obj, AccessExclusiveLock);
-       ev_relid = RelationGetRelid(event_relation);
+       event_relation = heap_open(event_relid, AccessExclusiveLock);
 
        /*
         * Check user has permission to apply rules to this relation.
         */
-       if (!pg_class_ownercheck(ev_relid, GetUserId()))
+       if (!pg_class_ownercheck(event_relid, GetUserId()))
                aclcheck_error(ACLCHECK_NOT_OWNER, ACL_KIND_CLASS,
                                           RelationGetRelationName(event_relation));
 
@@ -352,12 +355,13 @@ DefineQueryRewrite(char *rulename,
                         * truncated.
                         */
                        if (strncmp(rulename, "_RET", 4) != 0 ||
-                               strncmp(rulename + 4, event_obj->relname,
+                               strncmp(rulename + 4, RelationGetRelationName(event_relation),
                                                NAMEDATALEN - 4 - 4) != 0)
                                ereport(ERROR,
                                                (errcode(ERRCODE_INVALID_OBJECT_DEFINITION),
                                                 errmsg("view rule for \"%s\" must be named \"%s\"",
-                                                               event_obj->relname, ViewSelectRuleName)));
+                                                               RelationGetRelationName(event_relation),
+                                                               ViewSelectRuleName)));
                        rulename = pstrdup(ViewSelectRuleName);
                }
 
@@ -377,27 +381,27 @@ DefineQueryRewrite(char *rulename,
                                ereport(ERROR,
                                                (errcode(ERRCODE_OBJECT_NOT_IN_PREREQUISITE_STATE),
                                                 errmsg("could not convert table \"%s\" to a view because it is not empty",
-                                                               event_obj->relname)));
+                                                               RelationGetRelationName(event_relation))));
                        heap_endscan(scanDesc);
 
                        if (event_relation->rd_rel->reltriggers != 0)
                                ereport(ERROR,
                                                (errcode(ERRCODE_OBJECT_NOT_IN_PREREQUISITE_STATE),
                                                 errmsg("could not convert table \"%s\" to a view because it has triggers",
-                                                               event_obj->relname),
+                                                               RelationGetRelationName(event_relation)),
                                                 errhint("In particular, the table cannot be involved in any foreign key relationships.")));
 
                        if (event_relation->rd_rel->relhasindex)
                                ereport(ERROR,
                                                (errcode(ERRCODE_OBJECT_NOT_IN_PREREQUISITE_STATE),
                                                 errmsg("could not convert table \"%s\" to a view because it has indexes",
-                                                               event_obj->relname)));
+                                                               RelationGetRelationName(event_relation))));
 
                        if (event_relation->rd_rel->relhassubclass)
                                ereport(ERROR,
                                                (errcode(ERRCODE_OBJECT_NOT_IN_PREREQUISITE_STATE),
                                                 errmsg("could not convert table \"%s\" to a view because it has child tables",
-                                                               event_obj->relname)));
+                                                               RelationGetRelationName(event_relation))));
 
                        RelisBecomingView = true;
                }
@@ -449,7 +453,7 @@ DefineQueryRewrite(char *rulename,
        {
                ruleId = InsertRule(rulename,
                                                        event_type,
-                                                       ev_relid,
+                                                       event_relid,
                                                        event_attno,
                                                        is_instead,
                                                        event_qual,
@@ -465,7 +469,7 @@ DefineQueryRewrite(char *rulename,
                 * backends (including me!) to update relcache entries with the new
                 * rule.
                 */
-               SetRelationRuleStatus(ev_relid, true, RelisBecomingView);
+               SetRelationRuleStatus(event_relid, true, RelisBecomingView);
        }
 
        /*
@@ -701,7 +705,7 @@ EnableDisableRule(Relation rel, const char *rulename,
 /*
  * Rename an existing rewrite rule.
  *
- * This is unused code at the moment.
+ * This is unused code at the moment.  Note that it lacks a permissions check.
  */
 #ifdef NOT_USED
 void

diff --git a/src/include/rewrite/rewriteDefine.h b/src/include/rewrite/rewriteDefine.h
index d2b779061138cec06382d05a28093cf9d363e3f8..f4b0d8ac8c07216382c76fb9341db8e4cb690bd5 100644 (file)
--- a/src/include/rewrite/rewriteDefine.h
+++ b/src/include/rewrite/rewriteDefine.h
@@ -24,7 +24,7 @@
 extern void DefineRule(RuleStmt *stmt, const char *queryString);
 
 extern void DefineQueryRewrite(char *rulename,
-                                  RangeVar *event_obj,
+                                  Oid event_relid,
                                   Node *event_qual,
                                   CmdType event_type,
                                   bool is_instead,