Fix bugs in manipulation of large objects.

In v16 and up (since commit afbfc0298), large object ownership
checking has been broken because object_ownercheck() didn't take care
of the discrepancy between our object-address representation of large
objects (classId == LargeObjectRelationId) and the catalog where their
ownership info is actually stored (LargeObjectMetadataRelationId).
This resulted in failures such as "unrecognized class ID: 2613"
when trying to update blob properties as a non-superuser.

Poking around for related bugs, I found that AlterObjectOwner_internal
would pass the wrong classId to the PostAlterHook in the no-op code
path where the large object already has the desired owner.  Also,
recordExtObjInitPriv checked for the wrong classId; that bug is only
latent because the stanza is dead code anyway, but as long as we're
carrying it around it should be less wrong.  These bugs are quite old.

In HEAD, we can reduce the scope for future bugs of this ilk by
changing AlterObjectOwner_internal's API to let the translation happen
inside that function, rather than requiring callers to know about it.

A more bulletproof fix, perhaps, would be to start using
LargeObjectMetadataRelationId as the dependency and object-address
classId for blobs.  However that has substantial risk of breaking
third-party code; even within our own code, it'd create hassles
for pg_dump which would have to cope with a version-dependent
representation.  For now, keep the status quo.

Discussion: https://postgr.es/m/2650449.1702497209@sss.pgh.pa.us

diff --git a/src/backend/catalog/aclchk.c b/src/backend/catalog/aclchk.c
index 3ce6c09b44d5a44d657281b16e18dd8abb86a02f..17461a918906ff11a268a98b3fbf539b589cd0b5 100644 (file)
--- a/src/backend/catalog/aclchk.c
+++ b/src/backend/catalog/aclchk.c
@@ -4103,9 +4103,14 @@ object_ownercheck(Oid classid, Oid objectid, Oid roleid)
        if (superuser_arg(roleid))
                return true;
 
+       /* For large objects, the catalog to consult is pg_largeobject_metadata */
+       if (classid == LargeObjectRelationId)
+               classid = LargeObjectMetadataRelationId;
+
        cacheid = get_object_catcache_oid(classid);
        if (cacheid != -1)
        {
+               /* we can get the object's tuple from the syscache */
                HeapTuple       tuple;
 
                tuple = SearchSysCache1(cacheid, ObjectIdGetDatum(objectid));
@@ -4122,7 +4127,6 @@ object_ownercheck(Oid classid, Oid objectid, Oid roleid)
        else
        {
                /* for catalogs without an appropriate syscache */
-
                Relation        rel;
                ScanKeyData entry[1];
                SysScanDesc scan;
@@ -4442,9 +4446,9 @@ recordExtObjInitPriv(Oid objoid, Oid classoid)
 
                ReleaseSysCache(tuple);
        }
-       /* pg_largeobject_metadata */
-       else if (classoid == LargeObjectMetadataRelationId)
+       else if (classoid == LargeObjectRelationId)
        {
+               /* For large objects, we must consult pg_largeobject_metadata */
                Datum           aclDatum;
                bool            isNull;
                HeapTuple       tuple;

diff --git a/src/backend/catalog/pg_shdepend.c b/src/backend/catalog/pg_shdepend.c
index 8f09c632f96483577294eee3f36024e2624094e9..f9580fc8c4ec260fb7369c44c4406d29294e653f 100644 (file)
--- a/src/backend/catalog/pg_shdepend.c
+++ b/src/backend/catalog/pg_shdepend.c
@@ -1614,20 +1614,9 @@ shdepReassignOwned(List *roleids, Oid newrole)
                                case DatabaseRelationId:
                                case TSConfigRelationId:
                                case TSDictionaryRelationId:
-                                       {
-                                               Oid                     classId = sdepForm->classid;
-                                               Relation        catalog;
-
-                                               if (classId == LargeObjectRelationId)
-                                                       classId = LargeObjectMetadataRelationId;
-
-                                               catalog = table_open(classId, RowExclusiveLock);
-
-                                               AlterObjectOwner_internal(catalog, sdepForm->objid,
-                                                                                                 newrole);
-
-                                               table_close(catalog, NoLock);
-                                       }
+                                       AlterObjectOwner_internal(sdepForm->classid,
+                                                                                         sdepForm->objid,
+                                                                                         newrole);
                                        break;
 
                                default:

diff --git a/src/backend/commands/alter.c b/src/backend/commands/alter.c
index ff8d003876f84bb2def66efaa014bb40e2ca76f0..e9a344d03e33c9ca53ed09169c920c36874be146 100644 (file)
--- a/src/backend/commands/alter.c
+++ b/src/backend/commands/alter.c
@@ -918,9 +918,7 @@ ExecAlterOwnerStmt(AlterOwnerStmt *stmt)
                case OBJECT_TSDICTIONARY:
                case OBJECT_TSCONFIGURATION:
                        {
-                               Relation        catalog;
                                Relation        relation;
-                               Oid                     classId;
                                ObjectAddress address;
 
                                address = get_object_address(stmt->objectType,
@@ -929,20 +927,9 @@ ExecAlterOwnerStmt(AlterOwnerStmt *stmt)
                                                                                         AccessExclusiveLock,
                                                                                         false);
                                Assert(relation == NULL);
-                               classId = address.classId;
 
-                               /*
-                                * XXX - get_object_address returns Oid of pg_largeobject
-                                * catalog for OBJECT_LARGEOBJECT because of historical
-                                * reasons.  Fix up it here.
-                                */
-                               if (classId == LargeObjectRelationId)
-                                       classId = LargeObjectMetadataRelationId;
-
-                               catalog = table_open(classId, RowExclusiveLock);
-
-                               AlterObjectOwner_internal(catalog, address.objectId, newowner);
-                               table_close(catalog, RowExclusiveLock);
+                               AlterObjectOwner_internal(address.classId, address.objectId,
+                                                                                 newowner);
 
                                return address;
                        }
@@ -960,25 +947,32 @@ ExecAlterOwnerStmt(AlterOwnerStmt *stmt)
  * cases (won't work for tables, nor other cases where we need to do more than
  * change the ownership column of a single catalog entry).
  *
- * rel: catalog relation containing object (RowExclusiveLock'd by caller)
+ * classId: OID of catalog containing object
  * objectId: OID of object to change the ownership of
  * new_ownerId: OID of new object owner
+ *
+ * This will work on large objects, but we have to beware of the fact that
+ * classId isn't the OID of the catalog to modify in that case.
  */
 void
-AlterObjectOwner_internal(Relation rel, Oid objectId, Oid new_ownerId)
+AlterObjectOwner_internal(Oid classId, Oid objectId, Oid new_ownerId)
 {
-       Oid                     classId = RelationGetRelid(rel);
-       AttrNumber      Anum_oid = get_object_attnum_oid(classId);
-       AttrNumber      Anum_owner = get_object_attnum_owner(classId);
-       AttrNumber      Anum_namespace = get_object_attnum_namespace(classId);
-       AttrNumber      Anum_acl = get_object_attnum_acl(classId);
-       AttrNumber      Anum_name = get_object_attnum_name(classId);
+       /* For large objects, the catalog to modify is pg_largeobject_metadata */
+       Oid                     catalogId = (classId == LargeObjectRelationId) ? LargeObjectMetadataRelationId : classId;
+       AttrNumber      Anum_oid = get_object_attnum_oid(catalogId);
+       AttrNumber      Anum_owner = get_object_attnum_owner(catalogId);
+       AttrNumber      Anum_namespace = get_object_attnum_namespace(catalogId);
+       AttrNumber      Anum_acl = get_object_attnum_acl(catalogId);
+       AttrNumber      Anum_name = get_object_attnum_name(catalogId);
+       Relation        rel;
        HeapTuple       oldtup;
        Datum           datum;
        bool            isnull;
        Oid                     old_ownerId;
        Oid                     namespaceId = InvalidOid;
 
+       rel = table_open(catalogId, RowExclusiveLock);
+
        oldtup = get_catalog_object_by_oid(rel, Anum_oid, objectId);
        if (oldtup == NULL)
                elog(ERROR, "cache lookup failed for object %u of catalog \"%s\"",
@@ -1026,7 +1020,8 @@ AlterObjectOwner_internal(Relation rel, Oid objectId, Oid new_ownerId)
                                        snprintf(namebuf, sizeof(namebuf), "%u", objectId);
                                        objname = namebuf;
                                }
-                               aclcheck_error(ACLCHECK_NOT_OWNER, get_object_type(classId, objectId),
+                               aclcheck_error(ACLCHECK_NOT_OWNER,
+                                                          get_object_type(catalogId, objectId),
                                                           objname);
                        }
                        /* Must be able to become new owner */
@@ -1079,8 +1074,6 @@ AlterObjectOwner_internal(Relation rel, Oid objectId, Oid new_ownerId)
                CatalogTupleUpdate(rel, &newtup->t_self, newtup);
 
                /* Update owner dependency reference */
-               if (classId == LargeObjectMetadataRelationId)
-                       classId = LargeObjectRelationId;
                changeDependencyOnOwner(classId, objectId, new_ownerId);
 
                /* Release memory */
@@ -1089,5 +1082,8 @@ AlterObjectOwner_internal(Relation rel, Oid objectId, Oid new_ownerId)
                pfree(replaces);
        }
 
+       /* Note the post-alter hook gets classId not catalogId */
        InvokeObjectPostAlterHook(classId, objectId, 0);
+
+       table_close(rel, RowExclusiveLock);
 }

diff --git a/src/backend/libpq/be-fsstubs.c b/src/backend/libpq/be-fsstubs.c
index d189044a4fc2cc916178963011112209ba9a8d9e..230c65753205ba234c15dfaddc01476ca43d2a15 100644 (file)
--- a/src/backend/libpq/be-fsstubs.c
+++ b/src/backend/libpq/be-fsstubs.c
@@ -43,7 +43,7 @@
 #include <unistd.h>
 
 #include "access/xact.h"
-#include "catalog/pg_largeobject_metadata.h"
+#include "catalog/pg_largeobject.h"
 #include "libpq/be-fsstubs.h"
 #include "libpq/libpq-fs.h"
 #include "miscadmin.h"
@@ -323,7 +323,7 @@ be_lo_unlink(PG_FUNCTION_ARGS)
         * relevant FDs.
         */
        if (!lo_compat_privileges &&
-               !object_ownercheck(LargeObjectMetadataRelationId, lobjId, GetUserId()))
+               !object_ownercheck(LargeObjectRelationId, lobjId, GetUserId()))
                ereport(ERROR,
                                (errcode(ERRCODE_INSUFFICIENT_PRIVILEGE),
                                 errmsg("must be owner of large object %u", lobjId)));

diff --git a/src/backend/storage/large_object/inv_api.c b/src/backend/storage/large_object/inv_api.c
index cc9c335a92d864c84593c4380383f776cb8da2b3..a35f1dfeb9f8a9fb5ecf9a1f50ea8cce4d6b7751 100644 (file)
--- a/src/backend/storage/large_object/inv_api.c
+++ b/src/backend/storage/large_object/inv_api.c
@@ -221,11 +221,10 @@ inv_create(Oid lobjId)
        /*
         * dependency on the owner of largeobject
         *
-        * The reason why we use LargeObjectRelationId instead of
-        * LargeObjectMetadataRelationId here is to provide backward compatibility
-        * to the applications which utilize a knowledge about internal layout of
-        * system catalogs. OID of pg_largeobject_metadata and loid of
-        * pg_largeobject are same value, so there are no actual differences here.
+        * Note that LO dependencies are recorded using classId
+        * LargeObjectRelationId for backwards-compatibility reasons.  Using
+        * LargeObjectMetadataRelationId instead would simplify matters for the
+        * backend, but it'd complicate pg_dump and possibly break other clients.
         */
        recordDependencyOnOwner(LargeObjectRelationId,
                                                        lobjId_new, GetUserId());

diff --git a/src/include/commands/alter.h b/src/include/commands/alter.h
index ae79968fadda7e4140a00e063aca2c58de9330b7..5ddeaba8f0d9acbb77f757b79fb6a52135789197 100644 (file)
--- a/src/include/commands/alter.h
+++ b/src/include/commands/alter.h
@@ -17,7 +17,6 @@
 #include "catalog/dependency.h"
 #include "catalog/objectaddress.h"
 #include "nodes/parsenodes.h"
-#include "utils/relcache.h"
 
 extern ObjectAddress ExecRenameStmt(RenameStmt *stmt);
 
@@ -29,7 +28,7 @@ extern Oid    AlterObjectNamespace_oid(Oid classId, Oid objid, Oid nspOid,
                                                                         ObjectAddresses *objsMoved);
 
 extern ObjectAddress ExecAlterOwnerStmt(AlterOwnerStmt *stmt);
-extern void AlterObjectOwner_internal(Relation rel, Oid objectId,
+extern void AlterObjectOwner_internal(Oid classId, Oid objectId,
                                                                          Oid new_ownerId);
 
 #endif                                                 /* ALTER_H */

diff --git a/src/test/regress/expected/largeobject.out b/src/test/regress/expected/largeobject.out
index bdcede6728e8acd0d248c0d4dcac41fac1b5a825..4921dd79aeec1716ceaba9334612dfefd8575146 100644 (file)
--- a/src/test/regress/expected/largeobject.out
+++ b/src/test/regress/expected/largeobject.out
@@ -6,7 +6,7 @@
 \getenv abs_builddir PG_ABS_BUILDDIR
 -- ensure consistent test output regardless of the default bytea format
 SET bytea_output TO escape;
--- Test ALTER LARGE OBJECT OWNER, GRANT, COMMENT
+-- Test ALTER LARGE OBJECT OWNER
 CREATE ROLE regress_lo_user;
 SELECT lo_create(42);
  lo_create 
@@ -15,8 +15,11 @@ SELECT lo_create(42);
 (1 row)
 
 ALTER LARGE OBJECT 42 OWNER TO regress_lo_user;
+-- Test GRANT, COMMENT as non-superuser
+SET SESSION AUTHORIZATION regress_lo_user;
 GRANT SELECT ON LARGE OBJECT 42 TO public;
 COMMENT ON LARGE OBJECT 42 IS 'the ultimate answer';
+RESET SESSION AUTHORIZATION;
 -- Test psql's \lo_list et al (we assume no other LOs exist yet)
 \lo_list
                Large objects

diff --git a/src/test/regress/expected/largeobject_1.out b/src/test/regress/expected/largeobject_1.out
index d700910c35962455132b018a73129b6120ad3df8..7172ddb39bb293418e209d5c4f2128920520eefb 100644 (file)
--- a/src/test/regress/expected/largeobject_1.out
+++ b/src/test/regress/expected/largeobject_1.out
@@ -6,7 +6,7 @@
 \getenv abs_builddir PG_ABS_BUILDDIR
 -- ensure consistent test output regardless of the default bytea format
 SET bytea_output TO escape;
--- Test ALTER LARGE OBJECT OWNER, GRANT, COMMENT
+-- Test ALTER LARGE OBJECT OWNER
 CREATE ROLE regress_lo_user;
 SELECT lo_create(42);
  lo_create 
@@ -15,8 +15,11 @@ SELECT lo_create(42);
 (1 row)
 
 ALTER LARGE OBJECT 42 OWNER TO regress_lo_user;
+-- Test GRANT, COMMENT as non-superuser
+SET SESSION AUTHORIZATION regress_lo_user;
 GRANT SELECT ON LARGE OBJECT 42 TO public;
 COMMENT ON LARGE OBJECT 42 IS 'the ultimate answer';
+RESET SESSION AUTHORIZATION;
 -- Test psql's \lo_list et al (we assume no other LOs exist yet)
 \lo_list
                Large objects

diff --git a/src/test/regress/sql/largeobject.sql b/src/test/regress/sql/largeobject.sql
index 800e4fcc6a2b2874207b60b8b99bb12475274fa6..a4aee02e3a4ea7c369d7d684354eb8f9627f568f 100644 (file)
--- a/src/test/regress/sql/largeobject.sql
+++ b/src/test/regress/sql/largeobject.sql
@@ -9,13 +9,19 @@
 -- ensure consistent test output regardless of the default bytea format
 SET bytea_output TO escape;
 
--- Test ALTER LARGE OBJECT OWNER, GRANT, COMMENT
+-- Test ALTER LARGE OBJECT OWNER
 CREATE ROLE regress_lo_user;
 SELECT lo_create(42);
 ALTER LARGE OBJECT 42 OWNER TO regress_lo_user;
+
+-- Test GRANT, COMMENT as non-superuser
+SET SESSION AUTHORIZATION regress_lo_user;
+
 GRANT SELECT ON LARGE OBJECT 42 TO public;
 COMMENT ON LARGE OBJECT 42 IS 'the ultimate answer';
 
+RESET SESSION AUTHORIZATION;
+
 -- Test psql's \lo_list et al (we assume no other LOs exist yet)
 \lo_list
 \lo_list+