database/postgres
1
0
Fork 0
mirror of https://github.com/postgres/postgres.git synced 2025-07-07 00:36:50 +03:00
Code Activity

Fix bugs in manipulation of large objects.

Browse Source 
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
This commit is contained in:
Tom Lane
2023-12-15 13:55:05 -05:00
parent 0e917508b8
commit 59bd34c2fa
9 changed files with 54 additions and 55 deletions
Download Patch File Download Diff File Expand all files Collapse all files

48
src/backend/commands/alter.c
View File

@ -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);
}