Fix corner-case failures in has_foo_privilege() family of functions.

The variants of these functions that take numeric inputs (OIDs or
column numbers) are supposed to return NULL rather than failing
on bad input; this rule reduces problems with snapshot skew when
queries apply the functions to all rows of a catalog.

has_column_privilege() had careless handling of the case where the
table OID didn't exist.  You might get something like this:
	select has_column_privilege(9999,'nosuchcol','select');
	ERROR:  column "nosuchcol" of relation "(null)" does not exist
or you might get a crash, depending on the platform's printf's response
to a null string pointer.

In addition, while applying the column-number variant to a dropped
column returned NULL as desired, applying the column-name variant
did not:
	select has_column_privilege('mytable','........pg.dropped.2........','select');
	ERROR:  column "........pg.dropped.2........" of relation "mytable" does not exist
It seems better to make this case return NULL as well.

Also, the OID-accepting variants of has_foreign_data_wrapper_privilege,
has_server_privilege, and has_tablespace_privilege didn't follow the
principle of returning NULL for nonexistent OIDs.  Superusers got TRUE,
everybody else got an error.

Per investigation of Jaime Casanova's report of a new crash in HEAD.
These behaviors have been like this for a long time, so back-patch to
all supported branches.

Patch by me; thanks to Stephen Frost for discussion and review

Discussion: https://postgr.es/m/CAJGNTeP=-6Gyqq5TN9OvYEydi7Fv1oGyYj650LGTnW44oAzYCg@mail.gmail.com

diff --git a/src/backend/utils/adt/acl.c b/src/backend/utils/adt/acl.c
index d5285e259992ff54006911018becbd5b698de19e..c5f7918440bdb159d1845d5f020d3975bf4365df 100644 (file)
--- a/src/backend/utils/adt/acl.c
+++ b/src/backend/utils/adt/acl.c
@@ -2447,8 +2447,12 @@ has_any_column_privilege_id_id(PG_FUNCTION_ARGS)
  *
  *     The result is a boolean value: true if user has the indicated
  *     privilege, false if not.  The variants that take a relation OID
- *     and an integer attnum return NULL (rather than throwing an error)
- *     if the column doesn't exist or is dropped.
+ *     return NULL (rather than throwing an error) if that relation OID
+ *     doesn't exist.  Likewise, the variants that take an integer attnum
+ *     return NULL (rather than throwing an error) if there is no such
+ *     pg_attribute entry.  All variants return NULL if an attisdropped
+ *     column is selected.  These rules are meant to avoid unnecessary
+ *     failures in queries that scan pg_attribute.
  */
 
 /*
@@ -2465,6 +2469,12 @@ column_privilege_check(Oid tableoid, AttrNumber attnum,
    HeapTuple   attTuple;
    Form_pg_attribute attributeForm;
 
+   /*
+    * If convert_column_name failed, we can just return -1 immediately.
+    */
+   if (attnum == InvalidAttrNumber)
+       return -1;
+
    /*
     * First check if we have the privilege at the table level.  We check
     * existence of the pg_class row before risking calling pg_class_aclcheck.
@@ -2826,21 +2836,59 @@ has_column_privilege_id_attnum(PG_FUNCTION_ARGS)
 
 /*
  * Given a table OID and a column name expressed as a string, look it up
- * and return the column number
+ * and return the column number.  Returns InvalidAttrNumber in cases
+ * where caller should return NULL instead of failing.
  */
 static AttrNumber
 convert_column_name(Oid tableoid, text *column)
 {
-   AttrNumber  attnum;
    char       *colname;
+   HeapTuple   attTuple;
+   AttrNumber  attnum;
 
    colname = text_to_cstring(column);
-   attnum = get_attnum(tableoid, colname);
-   if (attnum == InvalidAttrNumber)
-       ereport(ERROR,
-               (errcode(ERRCODE_UNDEFINED_COLUMN),
-                errmsg("column \"%s\" of relation \"%s\" does not exist",
-                       colname, get_rel_name(tableoid))));
+
+   /*
+    * We don't use get_attnum() here because it will report that dropped
+    * columns don't exist.  We need to treat dropped columns differently from
+    * nonexistent columns.
+    */
+   attTuple = SearchSysCache2(ATTNAME,
+                              ObjectIdGetDatum(tableoid),
+                              CStringGetDatum(colname));
+   if (HeapTupleIsValid(attTuple))
+   {
+       Form_pg_attribute attributeForm;
+
+       attributeForm = (Form_pg_attribute) GETSTRUCT(attTuple);
+       /* We want to return NULL for dropped columns */
+       if (attributeForm->attisdropped)
+           attnum = InvalidAttrNumber;
+       else
+           attnum = attributeForm->attnum;
+       ReleaseSysCache(attTuple);
+   }
+   else
+   {
+       char       *tablename = get_rel_name(tableoid);
+
+       /*
+        * If the table OID is bogus, or it's just been dropped, we'll get
+        * NULL back.  In such cases we want has_column_privilege to return
+        * NULL too, so just return InvalidAttrNumber.
+        */
+       if (tablename != NULL)
+       {
+           /* tableoid exists, colname does not, so throw error */
+           ereport(ERROR,
+                   (errcode(ERRCODE_UNDEFINED_COLUMN),
+                    errmsg("column \"%s\" of relation \"%s\" does not exist",
+                           colname, tablename)));
+       }
+       /* tableoid doesn't exist, so act like attisdropped case */
+       attnum = InvalidAttrNumber;
+   }
+
    pfree(colname);
    return attnum;
 }
@@ -3144,6 +3192,9 @@ has_foreign_data_wrapper_privilege_name_id(PG_FUNCTION_ARGS)
    roleid = get_role_oid_or_public(NameStr(*username));
    mode = convert_foreign_data_wrapper_priv_string(priv_type_text);
 
+   if (!SearchSysCacheExists1(FOREIGNDATAWRAPPEROID, ObjectIdGetDatum(fdwid)))
+       PG_RETURN_NULL();
+
    aclresult = pg_foreign_data_wrapper_aclcheck(fdwid, roleid, mode);
 
    PG_RETURN_BOOL(aclresult == ACLCHECK_OK);
@@ -3167,6 +3218,9 @@ has_foreign_data_wrapper_privilege_id(PG_FUNCTION_ARGS)
    roleid = GetUserId();
    mode = convert_foreign_data_wrapper_priv_string(priv_type_text);
 
+   if (!SearchSysCacheExists1(FOREIGNDATAWRAPPEROID, ObjectIdGetDatum(fdwid)))
+       PG_RETURN_NULL();
+
    aclresult = pg_foreign_data_wrapper_aclcheck(fdwid, roleid, mode);
 
    PG_RETURN_BOOL(aclresult == ACLCHECK_OK);
@@ -3211,6 +3265,9 @@ has_foreign_data_wrapper_privilege_id_id(PG_FUNCTION_ARGS)
 
    mode = convert_foreign_data_wrapper_priv_string(priv_type_text);
 
+   if (!SearchSysCacheExists1(FOREIGNDATAWRAPPEROID, ObjectIdGetDatum(fdwid)))
+       PG_RETURN_NULL();
+
    aclresult = pg_foreign_data_wrapper_aclcheck(fdwid, roleid, mode);
 
    PG_RETURN_BOOL(aclresult == ACLCHECK_OK);
@@ -3910,6 +3967,9 @@ has_server_privilege_name_id(PG_FUNCTION_ARGS)
    roleid = get_role_oid_or_public(NameStr(*username));
    mode = convert_server_priv_string(priv_type_text);
 
+   if (!SearchSysCacheExists1(FOREIGNSERVEROID, ObjectIdGetDatum(serverid)))
+       PG_RETURN_NULL();
+
    aclresult = pg_foreign_server_aclcheck(serverid, roleid, mode);
 
    PG_RETURN_BOOL(aclresult == ACLCHECK_OK);
@@ -3933,6 +3993,9 @@ has_server_privilege_id(PG_FUNCTION_ARGS)
    roleid = GetUserId();
    mode = convert_server_priv_string(priv_type_text);
 
+   if (!SearchSysCacheExists1(FOREIGNSERVEROID, ObjectIdGetDatum(serverid)))
+       PG_RETURN_NULL();
+
    aclresult = pg_foreign_server_aclcheck(serverid, roleid, mode);
 
    PG_RETURN_BOOL(aclresult == ACLCHECK_OK);
@@ -3977,6 +4040,9 @@ has_server_privilege_id_id(PG_FUNCTION_ARGS)
 
    mode = convert_server_priv_string(priv_type_text);
 
+   if (!SearchSysCacheExists1(FOREIGNSERVEROID, ObjectIdGetDatum(serverid)))
+       PG_RETURN_NULL();
+
    aclresult = pg_foreign_server_aclcheck(serverid, roleid, mode);
 
    PG_RETURN_BOOL(aclresult == ACLCHECK_OK);
@@ -4092,6 +4158,9 @@ has_tablespace_privilege_name_id(PG_FUNCTION_ARGS)
    roleid = get_role_oid_or_public(NameStr(*username));
    mode = convert_tablespace_priv_string(priv_type_text);
 
+   if (!SearchSysCacheExists1(TABLESPACEOID, ObjectIdGetDatum(tablespaceoid)))
+       PG_RETURN_NULL();
+
    aclresult = pg_tablespace_aclcheck(tablespaceoid, roleid, mode);
 
    PG_RETURN_BOOL(aclresult == ACLCHECK_OK);
@@ -4115,6 +4184,9 @@ has_tablespace_privilege_id(PG_FUNCTION_ARGS)
    roleid = GetUserId();
    mode = convert_tablespace_priv_string(priv_type_text);
 
+   if (!SearchSysCacheExists1(TABLESPACEOID, ObjectIdGetDatum(tablespaceoid)))
+       PG_RETURN_NULL();
+
    aclresult = pg_tablespace_aclcheck(tablespaceoid, roleid, mode);
 
    PG_RETURN_BOOL(aclresult == ACLCHECK_OK);
@@ -4159,6 +4231,9 @@ has_tablespace_privilege_id_id(PG_FUNCTION_ARGS)
 
    mode = convert_tablespace_priv_string(priv_type_text);
 
+   if (!SearchSysCacheExists1(TABLESPACEOID, ObjectIdGetDatum(tablespaceoid)))
+       PG_RETURN_NULL();
+
    aclresult = pg_tablespace_aclcheck(tablespaceoid, roleid, mode);
 
    PG_RETURN_BOOL(aclresult == ACLCHECK_OK);

diff --git a/src/test/regress/expected/privileges.out b/src/test/regress/expected/privileges.out
index ac8968d24f51f22768666e639eef008846000ed4..aebc89c1b08431b980bdb5721e5cf599cab40bac 100644 (file)
--- a/src/test/regress/expected/privileges.out
+++ b/src/test/regress/expected/privileges.out
@@ -1132,6 +1132,63 @@ from (select oid from pg_class where relname = 'atest1') as t1;
  f
 (1 row)
 
+-- has_column_privilege function
+-- bad-input checks (as non-super-user)
+select has_column_privilege('pg_authid',NULL,'select');
+ has_column_privilege 
+----------------------
+ 
+(1 row)
+
+select has_column_privilege('pg_authid','nosuchcol','select');
+ERROR:  column "nosuchcol" of relation "pg_authid" does not exist
+select has_column_privilege(9999,'nosuchcol','select');
+ has_column_privilege 
+----------------------
+ 
+(1 row)
+
+select has_column_privilege(9999,99::int2,'select');
+ has_column_privilege 
+----------------------
+ 
+(1 row)
+
+select has_column_privilege('pg_authid',99::int2,'select');
+ has_column_privilege 
+----------------------
+ 
+(1 row)
+
+select has_column_privilege(9999,99::int2,'select');
+ has_column_privilege 
+----------------------
+ 
+(1 row)
+
+create temp table mytable(f1 int, f2 int, f3 int);
+alter table mytable drop column f2;
+select has_column_privilege('mytable','f2','select');
+ERROR:  column "f2" of relation "mytable" does not exist
+select has_column_privilege('mytable','........pg.dropped.2........','select');
+ has_column_privilege 
+----------------------
+ 
+(1 row)
+
+select has_column_privilege('mytable',2::int2,'select');
+ has_column_privilege 
+----------------------
+ t
+(1 row)
+
+revoke select on table mytable from regress_priv_user3;
+select has_column_privilege('mytable',2::int2,'select');
+ has_column_privilege 
+----------------------
+ 
+(1 row)
+
 -- Grant options
 SET SESSION AUTHORIZATION regress_priv_user1;
 CREATE TABLE atest4 (a int);

diff --git a/src/test/regress/sql/privileges.sql b/src/test/regress/sql/privileges.sql
index f7f3bbbeeb6897bc0d037db703006bc3e7329abc..0c0bba6cf7fe47acfc97f10981bb20305c7bc8ee 100644 (file)
--- a/src/test/regress/sql/privileges.sql
+++ b/src/test/regress/sql/privileges.sql
@@ -715,6 +715,23 @@ from (select oid from pg_class where relname = 'atest1') as t1;
 select has_table_privilege(t1.oid,'trigger')
 from (select oid from pg_class where relname = 'atest1') as t1;
 
+-- has_column_privilege function
+
+-- bad-input checks (as non-super-user)
+select has_column_privilege('pg_authid',NULL,'select');
+select has_column_privilege('pg_authid','nosuchcol','select');
+select has_column_privilege(9999,'nosuchcol','select');
+select has_column_privilege(9999,99::int2,'select');
+select has_column_privilege('pg_authid',99::int2,'select');
+select has_column_privilege(9999,99::int2,'select');
+
+create temp table mytable(f1 int, f2 int, f3 int);
+alter table mytable drop column f2;
+select has_column_privilege('mytable','f2','select');
+select has_column_privilege('mytable','........pg.dropped.2........','select');
+select has_column_privilege('mytable',2::int2,'select');
+revoke select on table mytable from regress_priv_user3;
+select has_column_privilege('mytable',2::int2,'select');
 
 -- Grant options