aboutsummaryrefslogtreecommitdiff
path: root/src
diff options
context:
space:
mode:
authorTom Lane <tgl@sss.pgh.pa.us>2023-12-15 13:55:05 -0500
committerTom Lane <tgl@sss.pgh.pa.us>2023-12-15 13:55:05 -0500
commit152bfc0af89de25400abe49205f1d9861a199b61 (patch)
tree72c78e3c44930e38f1a371da321584a2ee437f90 /src
parentdb69101a1d000d857a552e16e45f601adbb4dbc6 (diff)
downloadpostgresql-152bfc0af89de25400abe49205f1d9861a199b61.tar.gz
postgresql-152bfc0af89de25400abe49205f1d9861a199b61.zip
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
Diffstat (limited to 'src')
-rw-r--r--src/backend/catalog/aclchk.c10
-rw-r--r--src/backend/catalog/pg_shdepend.c4
-rw-r--r--src/backend/commands/alter.c22
-rw-r--r--src/backend/libpq/be-fsstubs.c4
-rw-r--r--src/backend/storage/large_object/inv_api.c9
-rw-r--r--src/test/regress/expected/largeobject.out5
-rw-r--r--src/test/regress/expected/largeobject_1.out5
-rw-r--r--src/test/regress/sql/largeobject.sql8
8 files changed, 50 insertions, 17 deletions
diff --git a/src/backend/catalog/aclchk.c b/src/backend/catalog/aclchk.c
index d1f5dcd8bed..a2aad09e6a0 100644
--- a/src/backend/catalog/aclchk.c
+++ b/src/backend/catalog/aclchk.c
@@ -3967,9 +3967,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));
@@ -3986,7 +3991,6 @@ object_ownercheck(Oid classid, Oid objectid, Oid roleid)
else
{
/* for catalogs without an appropriate syscache */
-
Relation rel;
ScanKeyData entry[1];
SysScanDesc scan;
@@ -4306,9 +4310,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 91c7f3426f9..ea41d9b7502 100644
--- a/src/backend/catalog/pg_shdepend.c
+++ b/src/backend/catalog/pg_shdepend.c
@@ -1618,6 +1618,10 @@ shdepReassignOwned(List *roleids, Oid newrole)
Oid classId = sdepForm->classid;
Relation catalog;
+ /*
+ * For large objects, the catalog to modify is
+ * pg_largeobject_metadata
+ */
if (classId == LargeObjectRelationId)
classId = LargeObjectMetadataRelationId;
diff --git a/src/backend/commands/alter.c b/src/backend/commands/alter.c
index e95dc31bde3..cbe02853b04 100644
--- a/src/backend/commands/alter.c
+++ b/src/backend/commands/alter.c
@@ -929,9 +929,8 @@ ExecAlterOwnerStmt(AlterOwnerStmt *stmt)
classId = address.classId;
/*
- * XXX - get_object_address returns Oid of pg_largeobject
- * catalog for OBJECT_LARGEOBJECT because of historical
- * reasons. Fix up it here.
+ * For large objects, the catalog to modify is
+ * pg_largeobject_metadata
*/
if (classId == LargeObjectRelationId)
classId = LargeObjectMetadataRelationId;
@@ -1075,9 +1074,14 @@ AlterObjectOwner_internal(Relation rel, Oid objectId, Oid new_ownerId)
/* Perform actual update */
CatalogTupleUpdate(rel, &newtup->t_self, newtup);
- /* Update owner dependency reference */
+ /*
+ * Update owner dependency reference. When working on a large object,
+ * we have to translate back to the OID conventionally used for LOs'
+ * classId.
+ */
if (classId == LargeObjectMetadataRelationId)
classId = LargeObjectRelationId;
+
changeDependencyOnOwner(classId, objectId, new_ownerId);
/* Release memory */
@@ -1085,6 +1089,16 @@ AlterObjectOwner_internal(Relation rel, Oid objectId, Oid new_ownerId)
pfree(nulls);
pfree(replaces);
}
+ else
+ {
+ /*
+ * No need to change anything. But when working on a large object, we
+ * have to translate back to the OID conventionally used for LOs'
+ * classId, or the post-alter hook (if any) will get confused.
+ */
+ if (classId == LargeObjectMetadataRelationId)
+ classId = LargeObjectRelationId;
+ }
InvokeObjectPostAlterHook(classId, objectId, 0);
}
diff --git a/src/backend/libpq/be-fsstubs.c b/src/backend/libpq/be-fsstubs.c
index d189044a4fc..230c6575320 100644
--- 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 84e543e7310..cab47f82fb6 100644
--- 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/test/regress/expected/largeobject.out b/src/test/regress/expected/largeobject.out
index bdcede6728e..4921dd79aee 100644
--- 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 d700910c359..7172ddb39bb 100644
--- 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 800e4fcc6a2..a4aee02e3a4 100644
--- 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+