public inbox for [email protected]
help / color / mirror / Atom feedFrom: Peter Eisentraut <[email protected]>
To: pgsql-hackers <[email protected]>
Subject: downgrade some aclchk.c errors to internal
Date: Fri, 20 Dec 2024 12:47:31 +0100
Message-ID: <[email protected]> (raw)
In aclchk.c, there are a few error messages that use ereport() but it
seems like they should be internal error messages. Moreover, they are
using get_object_class_descr(), which is only meant for internal errors.
(For example, it does not have translation support.) I dug through
this and it seems like these errors are indeed not or no longer
user-facing, so we can downgrade them to internal. See commit messages
in the attached patches for further explanations.
From b9a2945b367bc32f1a1dda421a72e176fecacdda Mon Sep 17 00:00:00 2001
From: Peter Eisentraut <[email protected]>
Date: Fri, 20 Dec 2024 10:10:19 +0100
Subject: [PATCH 1/2] Downgrade errors in object_ownercheck() to internal
The "does not exist" errors in object_ownership() were written as
ereport(), suggesting that they are user-facing. But no code path
except one can reach this function without first checking that the
object exists. If this were actually a user-facing error message,
then there would be some problems: get_object_class_descr() is meant
to be for internal errors only and does not support translation.
The one case that can reach this without first checking the object
existence is from be_lo_unlink(). (This makes some sense since large
objects are referred to by their OID directly.) In this one case, we
can add a line of code to check the object existence explicitly,
consistent with other LO code.
For the rest, downgrade the error messages to elog()s. The new
message wordings are the same as in DropObjectById().
---
src/backend/catalog/aclchk.c | 10 ++++------
src/backend/libpq/be-fsstubs.c | 5 +++++
2 files changed, 9 insertions(+), 6 deletions(-)
diff --git a/src/backend/catalog/aclchk.c b/src/backend/catalog/aclchk.c
index aaf96a965e4..840122dca44 100644
--- a/src/backend/catalog/aclchk.c
+++ b/src/backend/catalog/aclchk.c
@@ -4082,9 +4082,8 @@ object_ownercheck(Oid classid, Oid objectid, Oid roleid)
tuple = SearchSysCache1(cacheid, ObjectIdGetDatum(objectid));
if (!HeapTupleIsValid(tuple))
- ereport(ERROR,
- (errcode(ERRCODE_UNDEFINED_OBJECT),
- errmsg("%s with OID %u does not exist", get_object_class_descr(classid), objectid)));
+ elog(ERROR, "cache lookup failed for %s %u",
+ get_object_class_descr(classid), objectid);
ownerId = DatumGetObjectId(SysCacheGetAttrNotNull(cacheid,
tuple,
@@ -4113,9 +4112,8 @@ object_ownercheck(Oid classid, Oid objectid, Oid roleid)
tuple = systable_getnext(scan);
if (!HeapTupleIsValid(tuple))
- ereport(ERROR,
- (errcode(ERRCODE_UNDEFINED_OBJECT),
- errmsg("%s with OID %u does not exist", get_object_class_descr(classid), objectid)));
+ elog(ERROR, "could not find tuple for %s %u",
+ get_object_class_descr(classid), objectid);
ownerId = DatumGetObjectId(heap_getattr(tuple,
get_object_attnum_owner(classid),
diff --git a/src/backend/libpq/be-fsstubs.c b/src/backend/libpq/be-fsstubs.c
index 27d317dfdc0..66bec869cbc 100644
--- a/src/backend/libpq/be-fsstubs.c
+++ b/src/backend/libpq/be-fsstubs.c
@@ -317,6 +317,11 @@ be_lo_unlink(PG_FUNCTION_ARGS)
PreventCommandIfReadOnly("lo_unlink()");
+ if (!LargeObjectExists(lobjId))
+ ereport(ERROR,
+ (errcode(ERRCODE_UNDEFINED_OBJECT),
+ errmsg("large object %u does not exist", lobjId)));
+
/*
* Must be owner of the large object. It would be cleaner to check this
* in inv_drop(), but we want to throw the error before not after closing
base-commit: 8ac0021b6f10928a46b7f3d1b25bc21c0ac7f8c5
--
2.47.1
From 320a62cd669cc7a43cab950ed60f956984cd65b4 Mon Sep 17 00:00:00 2001
From: Peter Eisentraut <[email protected]>
Date: Fri, 20 Dec 2024 10:48:20 +0100
Subject: [PATCH 2/2] Downgrade error in object_aclmask_ext() to internal
The "does not exist" error in object_aclmask_ext() was written as
ereport(), suggesting that it is user-facing. But this has not been
true since commit 403ac226ddd. So we can downgrade this to a normal
"cache lookup failed" internal error. If this were actually a
user-facing error message, then there would be some problems:
get_object_class_descr() is meant to be for internal errors only and
does not support translation.
---
src/backend/catalog/aclchk.c | 10 ++--------
1 file changed, 2 insertions(+), 8 deletions(-)
diff --git a/src/backend/catalog/aclchk.c b/src/backend/catalog/aclchk.c
index 840122dca44..25b15027447 100644
--- a/src/backend/catalog/aclchk.c
+++ b/src/backend/catalog/aclchk.c
@@ -3004,10 +3004,6 @@ pg_aclmask(ObjectType objtype, Oid object_oid, AttrNumber attnum, Oid roleid,
* Exported routines for examining a user's privileges for various objects
*
* See aclmask() for a description of the common API for these functions.
- *
- * Note: we give lookup failure the full ereport treatment because the
- * has_xxx_privilege() family of functions allow users to pass any random
- * OID to these functions.
* ****************************************************************
*/
@@ -3074,10 +3070,8 @@ object_aclmask_ext(Oid classid, Oid objectid, Oid roleid,
return 0;
}
else
- ereport(ERROR,
- (errcode(ERRCODE_UNDEFINED_OBJECT),
- errmsg("%s with OID %u does not exist",
- get_object_class_descr(classid), objectid)));
+ elog(ERROR, "cache lookup failed for %s %u",
+ get_object_class_descr(classid), objectid);
}
ownerId = DatumGetObjectId(SysCacheGetAttrNotNull(cacheid,
--
2.47.1
Attachments:
[text/plain] 0001-Downgrade-errors-in-object_ownercheck-to-internal.patch (3.0K, ../[email protected]/2-0001-Downgrade-errors-in-object_ownercheck-to-internal.patch)
download | inline diff:
From b9a2945b367bc32f1a1dda421a72e176fecacdda Mon Sep 17 00:00:00 2001
From: Peter Eisentraut <[email protected]>
Date: Fri, 20 Dec 2024 10:10:19 +0100
Subject: [PATCH 1/2] Downgrade errors in object_ownercheck() to internal
The "does not exist" errors in object_ownership() were written as
ereport(), suggesting that they are user-facing. But no code path
except one can reach this function without first checking that the
object exists. If this were actually a user-facing error message,
then there would be some problems: get_object_class_descr() is meant
to be for internal errors only and does not support translation.
The one case that can reach this without first checking the object
existence is from be_lo_unlink(). (This makes some sense since large
objects are referred to by their OID directly.) In this one case, we
can add a line of code to check the object existence explicitly,
consistent with other LO code.
For the rest, downgrade the error messages to elog()s. The new
message wordings are the same as in DropObjectById().
---
src/backend/catalog/aclchk.c | 10 ++++------
src/backend/libpq/be-fsstubs.c | 5 +++++
2 files changed, 9 insertions(+), 6 deletions(-)
diff --git a/src/backend/catalog/aclchk.c b/src/backend/catalog/aclchk.c
index aaf96a965e4..840122dca44 100644
--- a/src/backend/catalog/aclchk.c
+++ b/src/backend/catalog/aclchk.c
@@ -4082,9 +4082,8 @@ object_ownercheck(Oid classid, Oid objectid, Oid roleid)
tuple = SearchSysCache1(cacheid, ObjectIdGetDatum(objectid));
if (!HeapTupleIsValid(tuple))
- ereport(ERROR,
- (errcode(ERRCODE_UNDEFINED_OBJECT),
- errmsg("%s with OID %u does not exist", get_object_class_descr(classid), objectid)));
+ elog(ERROR, "cache lookup failed for %s %u",
+ get_object_class_descr(classid), objectid);
ownerId = DatumGetObjectId(SysCacheGetAttrNotNull(cacheid,
tuple,
@@ -4113,9 +4112,8 @@ object_ownercheck(Oid classid, Oid objectid, Oid roleid)
tuple = systable_getnext(scan);
if (!HeapTupleIsValid(tuple))
- ereport(ERROR,
- (errcode(ERRCODE_UNDEFINED_OBJECT),
- errmsg("%s with OID %u does not exist", get_object_class_descr(classid), objectid)));
+ elog(ERROR, "could not find tuple for %s %u",
+ get_object_class_descr(classid), objectid);
ownerId = DatumGetObjectId(heap_getattr(tuple,
get_object_attnum_owner(classid),
diff --git a/src/backend/libpq/be-fsstubs.c b/src/backend/libpq/be-fsstubs.c
index 27d317dfdc0..66bec869cbc 100644
--- a/src/backend/libpq/be-fsstubs.c
+++ b/src/backend/libpq/be-fsstubs.c
@@ -317,6 +317,11 @@ be_lo_unlink(PG_FUNCTION_ARGS)
PreventCommandIfReadOnly("lo_unlink()");
+ if (!LargeObjectExists(lobjId))
+ ereport(ERROR,
+ (errcode(ERRCODE_UNDEFINED_OBJECT),
+ errmsg("large object %u does not exist", lobjId)));
+
/*
* Must be owner of the large object. It would be cleaner to check this
* in inv_drop(), but we want to throw the error before not after closing
base-commit: 8ac0021b6f10928a46b7f3d1b25bc21c0ac7f8c5
--
2.47.1
[text/plain] 0002-Downgrade-error-in-object_aclmask_ext-to-internal.patch (1.9K, ../[email protected]/3-0002-Downgrade-error-in-object_aclmask_ext-to-internal.patch)
download | inline diff:
From 320a62cd669cc7a43cab950ed60f956984cd65b4 Mon Sep 17 00:00:00 2001
From: Peter Eisentraut <[email protected]>
Date: Fri, 20 Dec 2024 10:48:20 +0100
Subject: [PATCH 2/2] Downgrade error in object_aclmask_ext() to internal
The "does not exist" error in object_aclmask_ext() was written as
ereport(), suggesting that it is user-facing. But this has not been
true since commit 403ac226ddd. So we can downgrade this to a normal
"cache lookup failed" internal error. If this were actually a
user-facing error message, then there would be some problems:
get_object_class_descr() is meant to be for internal errors only and
does not support translation.
---
src/backend/catalog/aclchk.c | 10 ++--------
1 file changed, 2 insertions(+), 8 deletions(-)
diff --git a/src/backend/catalog/aclchk.c b/src/backend/catalog/aclchk.c
index 840122dca44..25b15027447 100644
--- a/src/backend/catalog/aclchk.c
+++ b/src/backend/catalog/aclchk.c
@@ -3004,10 +3004,6 @@ pg_aclmask(ObjectType objtype, Oid object_oid, AttrNumber attnum, Oid roleid,
* Exported routines for examining a user's privileges for various objects
*
* See aclmask() for a description of the common API for these functions.
- *
- * Note: we give lookup failure the full ereport treatment because the
- * has_xxx_privilege() family of functions allow users to pass any random
- * OID to these functions.
* ****************************************************************
*/
@@ -3074,10 +3070,8 @@ object_aclmask_ext(Oid classid, Oid objectid, Oid roleid,
return 0;
}
else
- ereport(ERROR,
- (errcode(ERRCODE_UNDEFINED_OBJECT),
- errmsg("%s with OID %u does not exist",
- get_object_class_descr(classid), objectid)));
+ elog(ERROR, "cache lookup failed for %s %u",
+ get_object_class_descr(classid), objectid);
}
ownerId = DatumGetObjectId(SysCacheGetAttrNotNull(cacheid,
--
2.47.1
view thread (2+ messages)
reply
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Reply to all the recipients using the --to and --cc options:
reply via email
To: [email protected]
Cc: [email protected]
Subject: Re: downgrade some aclchk.c errors to internal
In-Reply-To: <[email protected]>
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
This inbox is served by agora; see mirroring instructions
for how to clone and mirror all data and code used for this inbox