From 419cc8add5fb81331efbc7ea8862e08b981b7762 Mon Sep 17 00:00:00 2001 From: Tom Lane Date: Tue, 2 Oct 2018 11:54:12 -0400 Subject: [PATCH] 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 --- src/backend/utils/adt/acl.c | 95 +++++++++++++++++++++--- src/test/regress/expected/privileges.out | 57 ++++++++++++++ src/test/regress/sql/privileges.sql | 17 +++++ 3 files changed, 159 insertions(+), 10 deletions(-) diff --git a/src/backend/utils/adt/acl.c b/src/backend/utils/adt/acl.c index a45e093de7..c22ede7b8b 100644 --- a/src/backend/utils/adt/acl.c +++ b/src/backend/utils/adt/acl.c @@ -2448,8 +2448,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. */ /* @@ -2466,6 +2470,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. @@ -2827,21 +2837,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; } @@ -3145,6 +3193,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); @@ -3168,6 +3219,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); @@ -3212,6 +3266,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); @@ -3911,6 +3968,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); @@ -3934,6 +3994,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); @@ -3978,6 +4041,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); @@ -4093,6 +4159,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); @@ -4116,6 +4185,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); @@ -4160,6 +4232,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 ac8968d24f..aebc89c1b0 100644 --- 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 f7f3bbbeeb..0c0bba6cf7 100644 --- 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