Skip to content

Commit 3d0f68d

Browse files
tglsfdctglsfdc
committed
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
1 parent 80810ca commit 3d0f68d

File tree

3 files changed

+159
-10
lines changed

3 files changed

+159
-10
lines changed

‎src/backend/utils/adt/acl.c‎

Lines changed: 85 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -2447,8 +2447,12 @@ has_any_column_privilege_id_id(PG_FUNCTION_ARGS)
24472447
*
24482448
* The result is a boolean value: true if user has the indicated
24492449
* privilege, false if not. The variants that take a relation OID
2450-
* and an integer attnum return NULL (rather than throwing an error)
2451-
* if the column doesn't exist or is dropped.
2450+
* return NULL (rather than throwing an error) if that relation OID
2451+
* doesn't exist. Likewise, the variants that take an integer attnum
2452+
* return NULL (rather than throwing an error) if there is no such
2453+
* pg_attribute entry. All variants return NULL if an attisdropped
2454+
* column is selected. These rules are meant to avoid unnecessary
2455+
* failures in queries that scan pg_attribute.
24522456
*/
24532457

24542458
/*
@@ -2465,6 +2469,12 @@ column_privilege_check(Oid tableoid, AttrNumber attnum,
24652469
HeapTuple attTuple;
24662470
Form_pg_attribute attributeForm;
24672471

2472+
/*
2473+
* If convert_column_name failed, we can just return -1 immediately.
2474+
*/
2475+
if (attnum == InvalidAttrNumber)
2476+
return -1;
2477+
24682478
/*
24692479
* First check if we have the privilege at the table level. We check
24702480
* existence of the pg_class row before risking calling pg_class_aclcheck.
@@ -2826,21 +2836,59 @@ has_column_privilege_id_attnum(PG_FUNCTION_ARGS)
28262836

28272837
/*
28282838
* Given a table OID and a column name expressed as a string, look it up
2829-
* and return the column number
2839+
* and return the column number. Returns InvalidAttrNumber in cases
2840+
* where caller should return NULL instead of failing.
28302841
*/
28312842
static AttrNumber
28322843
convert_column_name(Oid tableoid, text *column)
28332844
{
2834-
AttrNumber attnum;
28352845
char *colname;
2846+
HeapTuple attTuple;
2847+
AttrNumber attnum;
28362848

28372849
colname = text_to_cstring(column);
2838-
attnum = get_attnum(tableoid, colname);
2839-
if (attnum == InvalidAttrNumber)
2840-
ereport(ERROR,
2841-
(errcode(ERRCODE_UNDEFINED_COLUMN),
2842-
errmsg("column \"%s\" of relation \"%s\" does not exist",
2843-
colname, get_rel_name(tableoid))));
2850+
2851+
/*
2852+
* We don't use get_attnum() here because it will report that dropped
2853+
* columns don't exist. We need to treat dropped columns differently from
2854+
* nonexistent columns.
2855+
*/
2856+
attTuple = SearchSysCache2(ATTNAME,
2857+
ObjectIdGetDatum(tableoid),
2858+
CStringGetDatum(colname));
2859+
if (HeapTupleIsValid(attTuple))
2860+
{
2861+
Form_pg_attribute attributeForm;
2862+
2863+
attributeForm = (Form_pg_attribute) GETSTRUCT(attTuple);
2864+
/* We want to return NULL for dropped columns */
2865+
if (attributeForm->attisdropped)
2866+
attnum = InvalidAttrNumber;
2867+
else
2868+
attnum = attributeForm->attnum;
2869+
ReleaseSysCache(attTuple);
2870+
}
2871+
else
2872+
{
2873+
char *tablename = get_rel_name(tableoid);
2874+
2875+
/*
2876+
* If the table OID is bogus, or it's just been dropped, we'll get
2877+
* NULL back. In such cases we want has_column_privilege to return
2878+
* NULL too, so just return InvalidAttrNumber.
2879+
*/
2880+
if (tablename != NULL)
2881+
{
2882+
/* tableoid exists, colname does not, so throw error */
2883+
ereport(ERROR,
2884+
(errcode(ERRCODE_UNDEFINED_COLUMN),
2885+
errmsg("column \"%s\" of relation \"%s\" does not exist",
2886+
colname, tablename)));
2887+
}
2888+
/* tableoid doesn't exist, so act like attisdropped case */
2889+
attnum = InvalidAttrNumber;
2890+
}
2891+
28442892
pfree(colname);
28452893
return attnum;
28462894
}
@@ -3144,6 +3192,9 @@ has_foreign_data_wrapper_privilege_name_id(PG_FUNCTION_ARGS)
31443192
roleid = get_role_oid_or_public(NameStr(*username));
31453193
mode = convert_foreign_data_wrapper_priv_string(priv_type_text);
31463194

3195+
if (!SearchSysCacheExists1(FOREIGNDATAWRAPPEROID, ObjectIdGetDatum(fdwid)))
3196+
PG_RETURN_NULL();
3197+
31473198
aclresult = pg_foreign_data_wrapper_aclcheck(fdwid, roleid, mode);
31483199

31493200
PG_RETURN_BOOL(aclresult == ACLCHECK_OK);
@@ -3167,6 +3218,9 @@ has_foreign_data_wrapper_privilege_id(PG_FUNCTION_ARGS)
31673218
roleid = GetUserId();
31683219
mode = convert_foreign_data_wrapper_priv_string(priv_type_text);
31693220

3221+
if (!SearchSysCacheExists1(FOREIGNDATAWRAPPEROID, ObjectIdGetDatum(fdwid)))
3222+
PG_RETURN_NULL();
3223+
31703224
aclresult = pg_foreign_data_wrapper_aclcheck(fdwid, roleid, mode);
31713225

31723226
PG_RETURN_BOOL(aclresult == ACLCHECK_OK);
@@ -3211,6 +3265,9 @@ has_foreign_data_wrapper_privilege_id_id(PG_FUNCTION_ARGS)
32113265

32123266
mode = convert_foreign_data_wrapper_priv_string(priv_type_text);
32133267

3268+
if (!SearchSysCacheExists1(FOREIGNDATAWRAPPEROID, ObjectIdGetDatum(fdwid)))
3269+
PG_RETURN_NULL();
3270+
32143271
aclresult = pg_foreign_data_wrapper_aclcheck(fdwid, roleid, mode);
32153272

32163273
PG_RETURN_BOOL(aclresult == ACLCHECK_OK);
@@ -3910,6 +3967,9 @@ has_server_privilege_name_id(PG_FUNCTION_ARGS)
39103967
roleid = get_role_oid_or_public(NameStr(*username));
39113968
mode = convert_server_priv_string(priv_type_text);
39123969

3970+
if (!SearchSysCacheExists1(FOREIGNSERVEROID, ObjectIdGetDatum(serverid)))
3971+
PG_RETURN_NULL();
3972+
39133973
aclresult = pg_foreign_server_aclcheck(serverid, roleid, mode);
39143974

39153975
PG_RETURN_BOOL(aclresult == ACLCHECK_OK);
@@ -3933,6 +3993,9 @@ has_server_privilege_id(PG_FUNCTION_ARGS)
39333993
roleid = GetUserId();
39343994
mode = convert_server_priv_string(priv_type_text);
39353995

3996+
if (!SearchSysCacheExists1(FOREIGNSERVEROID, ObjectIdGetDatum(serverid)))
3997+
PG_RETURN_NULL();
3998+
39363999
aclresult = pg_foreign_server_aclcheck(serverid, roleid, mode);
39374000

39384001
PG_RETURN_BOOL(aclresult == ACLCHECK_OK);
@@ -3977,6 +4040,9 @@ has_server_privilege_id_id(PG_FUNCTION_ARGS)
39774040

39784041
mode = convert_server_priv_string(priv_type_text);
39794042

4043+
if (!SearchSysCacheExists1(FOREIGNSERVEROID, ObjectIdGetDatum(serverid)))
4044+
PG_RETURN_NULL();
4045+
39804046
aclresult = pg_foreign_server_aclcheck(serverid, roleid, mode);
39814047

39824048
PG_RETURN_BOOL(aclresult == ACLCHECK_OK);
@@ -4092,6 +4158,9 @@ has_tablespace_privilege_name_id(PG_FUNCTION_ARGS)
40924158
roleid = get_role_oid_or_public(NameStr(*username));
40934159
mode = convert_tablespace_priv_string(priv_type_text);
40944160

4161+
if (!SearchSysCacheExists1(TABLESPACEOID, ObjectIdGetDatum(tablespaceoid)))
4162+
PG_RETURN_NULL();
4163+
40954164
aclresult = pg_tablespace_aclcheck(tablespaceoid, roleid, mode);
40964165

40974166
PG_RETURN_BOOL(aclresult == ACLCHECK_OK);
@@ -4115,6 +4184,9 @@ has_tablespace_privilege_id(PG_FUNCTION_ARGS)
41154184
roleid = GetUserId();
41164185
mode = convert_tablespace_priv_string(priv_type_text);
41174186

4187+
if (!SearchSysCacheExists1(TABLESPACEOID, ObjectIdGetDatum(tablespaceoid)))
4188+
PG_RETURN_NULL();
4189+
41184190
aclresult = pg_tablespace_aclcheck(tablespaceoid, roleid, mode);
41194191

41204192
PG_RETURN_BOOL(aclresult == ACLCHECK_OK);
@@ -4159,6 +4231,9 @@ has_tablespace_privilege_id_id(PG_FUNCTION_ARGS)
41594231

41604232
mode = convert_tablespace_priv_string(priv_type_text);
41614233

4234+
if (!SearchSysCacheExists1(TABLESPACEOID, ObjectIdGetDatum(tablespaceoid)))
4235+
PG_RETURN_NULL();
4236+
41624237
aclresult = pg_tablespace_aclcheck(tablespaceoid, roleid, mode);
41634238

41644239
PG_RETURN_BOOL(aclresult == ACLCHECK_OK);

‎src/test/regress/expected/privileges.out‎

Lines changed: 57 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1132,6 +1132,63 @@ from (select oid from pg_class where relname = 'atest1') as t1;
11321132
f
11331133
(1 row)
11341134

1135+
-- has_column_privilege function
1136+
-- bad-input checks (as non-super-user)
1137+
select has_column_privilege('pg_authid',NULL,'select');
1138+
has_column_privilege
1139+
----------------------
1140+
1141+
(1 row)
1142+
1143+
select has_column_privilege('pg_authid','nosuchcol','select');
1144+
ERROR: column "nosuchcol" of relation "pg_authid" does not exist
1145+
select has_column_privilege(9999,'nosuchcol','select');
1146+
has_column_privilege
1147+
----------------------
1148+
1149+
(1 row)
1150+
1151+
select has_column_privilege(9999,99::int2,'select');
1152+
has_column_privilege
1153+
----------------------
1154+
1155+
(1 row)
1156+
1157+
select has_column_privilege('pg_authid',99::int2,'select');
1158+
has_column_privilege
1159+
----------------------
1160+
1161+
(1 row)
1162+
1163+
select has_column_privilege(9999,99::int2,'select');
1164+
has_column_privilege
1165+
----------------------
1166+
1167+
(1 row)
1168+
1169+
create temp table mytable(f1 int, f2 int, f3 int);
1170+
alter table mytable drop column f2;
1171+
select has_column_privilege('mytable','f2','select');
1172+
ERROR: column "f2" of relation "mytable" does not exist
1173+
select has_column_privilege('mytable','........pg.dropped.2........','select');
1174+
has_column_privilege
1175+
----------------------
1176+
1177+
(1 row)
1178+
1179+
select has_column_privilege('mytable',2::int2,'select');
1180+
has_column_privilege
1181+
----------------------
1182+
t
1183+
(1 row)
1184+
1185+
revoke select on table mytable from regress_priv_user3;
1186+
select has_column_privilege('mytable',2::int2,'select');
1187+
has_column_privilege
1188+
----------------------
1189+
1190+
(1 row)
1191+
11351192
-- Grant options
11361193
SET SESSION AUTHORIZATION regress_priv_user1;
11371194
CREATE TABLE atest4 (a int);

‎src/test/regress/sql/privileges.sql‎

Lines changed: 17 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -715,6 +715,23 @@ from (select oid from pg_class where relname = 'atest1') as t1;
715715
select has_table_privilege(t1.oid,'trigger')
716716
from (select oid from pg_class where relname = 'atest1') as t1;
717717

718+
-- has_column_privilege function
719+
720+
-- bad-input checks (as non-super-user)
721+
select has_column_privilege('pg_authid',NULL,'select');
722+
select has_column_privilege('pg_authid','nosuchcol','select');
723+
select has_column_privilege(9999,'nosuchcol','select');
724+
select has_column_privilege(9999,99::int2,'select');
725+
select has_column_privilege('pg_authid',99::int2,'select');
726+
select has_column_privilege(9999,99::int2,'select');
727+
728+
create temp table mytable(f1 int, f2 int, f3 int);
729+
alter table mytable drop column f2;
730+
select has_column_privilege('mytable','f2','select');
731+
select has_column_privilege('mytable','........pg.dropped.2........','select');
732+
select has_column_privilege('mytable',2::int2,'select');
733+
revoke select on table mytable from regress_priv_user3;
734+
select has_column_privilege('mytable',2::int2,'select');
718735

719736
-- Grant options
720737

0 commit comments

Comments
 (0)