Обсуждение: Avoid possible dereference null pointer (src/backend/catalog/pg_depend.c)

Hi.

Per Coverity.

     

2. returned_null: SearchSysCacheAttName returns NULL (checked 20 out of 21 times).

3. var_assigned: Assigning: ptup = NULL return value from SearchSysCacheAttName.

 964                ptup = SearchSysCacheAttName(relid, attname);

CID 1545986: (#1 of 1): Dereference null return value (NULL_RETURNS)

4. dereference: Dereferencing ptup, which is known to be NULL.

The functions SearchSysCacheAttNum and SearchSysCacheAttName,

need to have the result checked.

The commit 5091995, left an oversight.

Fixed by the patch attached, a change of style, unfortunately, was necessary.

Em qua., 22 de mai. de 2024 às 11:44, Ranier Vilela <ranier.vf@gmail.com> escreveu:

Hi.

Per Coverity.

     

2. returned_null: SearchSysCacheAttName returns NULL (checked 20 out of 21 times).

3. var_assigned: Assigning: ptup = NULL return value from SearchSysCacheAttName.

 964                ptup = SearchSysCacheAttName(relid, attname);

CID 1545986: (#1 of 1): Dereference null return value (NULL_RETURNS)

4. dereference: Dereferencing ptup, which is known to be NULL.

The functions SearchSysCacheAttNum and SearchSysCacheAttName,

need to have the result checked.

The commit 5091995, left an oversight.

Fixed by the patch attached, a change of style, unfortunately, was necessary.

v1 Attached, fix wrong column variable name in error report.

On Wed, May 22, 2024 at 03:28:48PM -0300, Ranier Vilela wrote:
> 1. Another concern is the function *get_partition_ancestors*,
> which may return NIL, which may affect *llast_oid*, which does not handle
> NIL entries.

Hm?  We already know in the code path that the relation we are dealing
with when calling get_partition_ancestors() *is* a partition thanks to
the check on relispartition, no?  In this case, calling
get_partition_ancestors() is valid and there should be a top-most
parent in any case all the time.  So I don't get the point of checking
get_partition_ancestors() for NIL-ness just for the sake of assuming
that it would be possible.

> 2. Is checking *relispartition* enough?
> There a function *check_rel_can_be_partition*
> (src/backend/utils/adt/partitionfuncs.c),
> which performs a much more robust check, would it be worth using it?
>
> With the v2 attached, 1 is handled, but, in this case,
> will it be the most correct?

Saying that, your point about the result of SearchSysCacheAttName not
checked if it is a valid tuple is right.  We paint errors in these
cases even if they should not happen as that's useful when it comes to
debugging, at least.

On Wed, May 22, 2024 at 03:28:48PM -0300, Ranier Vilela wrote:
> 1. Another concern is the function *get_partition_ancestors*,
> which may return NIL, which may affect *llast_oid*, which does not handle
> NIL entries.

Hm?  We already know in the code path that the relation we are dealing
with when calling get_partition_ancestors() *is* a partition thanks to
the check on relispartition, no?  In this case, calling
get_partition_ancestors() is valid and there should be a top-most
parent in any case all the time.  So I don't get the point of checking
get_partition_ancestors() for NIL-ness just for the sake of assuming
that it would be possible.

> 2. Is checking *relispartition* enough?
> There a function *check_rel_can_be_partition*
> (src/backend/utils/adt/partitionfuncs.c),
> which performs a much more robust check, would it be worth using it?
>
> With the v2 attached, 1 is handled, but, in this case,
> will it be the most correct?

Saying that, your point about the result of SearchSysCacheAttName not
checked if it is a valid tuple is right.  We paint errors in these
cases even if they should not happen as that's useful when it comes to
debugging, at least.

--
Michael

On Thu, May 23, 2024 at 5:52 AM Michael Paquier <michael@paquier.xyz> wrote:

On Wed, May 22, 2024 at 03:28:48PM -0300, Ranier Vilela wrote:
> 1. Another concern is the function *get_partition_ancestors*,
> which may return NIL, which may affect *llast_oid*, which does not handle
> NIL entries.

Hm?  We already know in the code path that the relation we are dealing
with when calling get_partition_ancestors() *is* a partition thanks to
the check on relispartition, no?  In this case, calling
get_partition_ancestors() is valid and there should be a top-most
parent in any case all the time.  So I don't get the point of checking
get_partition_ancestors() for NIL-ness just for the sake of assuming
that it would be possible.

+1.

 

> 2. Is checking *relispartition* enough?
> There a function *check_rel_can_be_partition*
> (src/backend/utils/adt/partitionfuncs.c),
> which performs a much more robust check, would it be worth using it?
>
> With the v2 attached, 1 is handled, but, in this case,
> will it be the most correct?

Saying that, your point about the result of SearchSysCacheAttName not
checked if it is a valid tuple is right.  We paint errors in these
cases even if they should not happen as that's useful when it comes to
debugging, at least.

I think an Assert would do instead of whole ereport().

The callers have already resolved attribute name to attribute number. Hence the attribute *should* exist in both partition as well as topmost partitioned table.

   relid = llast_oid(ancestors);
+
  ptup = SearchSysCacheAttName(relid, attname);
+ if (!HeapTupleIsValid(ptup))
+ ereport(ERROR,
+ (errcode(ERRCODE_UNDEFINED_COLUMN),
+ errmsg("column \"%s\" of relation \"%s\" does not exist",
+ attname, RelationGetRelationName(rel))));

We changed the relid from OID of partition to that of topmost partitioned table but didn't change rel; which still points to partition relation. We have to invoke relation_open() with new relid, in order to use rel in the error message. I don't think all that is worth it, unless we find a scenario when SearchSysCacheAttName() returns NULL.

On Thu, May 23, 2024 at 08:54:12AM -0300, Ranier Vilela wrote:
> All calls to functions like SearchSysCacheAttName, in the whole codebase,
> checks if returns are valid.
> It must be for a very strong reason, such a style.

Usually good practice, as I've outlined once upthread, because we do
expect the attributes to exist in this case.  Or if you want, an error
is better than a crash if a concurrent path causes this area to lead
to inconsistent lookups, which is something I've seen in the past
while hacking on my own stuff, or just fix other things causing
syscache lookup inconsistencies.  You'd be surprised to hear that
dropped attributes being mishandled is not that uncommon, especially
in out-of-core code, as one example.  FWIW, I don't see much a point
in using ereport(), the two checks ought to be elog()s pointing to an
internal error as these two errors should never happen.  Still, it is
a good idea to check that they never happen: aka an internal
error state is better than a crash if a problem arises.

> So, v3, implements it this way.

I don't understand the point behind the open/close of attrelation,
TBH.  That's not needed.

Except fot these two points, this is just moving the calls to make
sure that we have valid tuples from the syscache, which is a better
practice.  509199587df7 is recent enough that this should be fixed now
rather than later.

On Fri, May 24, 2024 at 11:58:51AM +0530, Ashutosh Bapat wrote:
> If we are looking for avoiding a segfault and get a message which helps
> debugging, using get_attname and get_attnum might be better options.
> get_attname throws an error. get_attnum doesn't throw an error and returns
> InvalidAttnum which won't return any valid identity sequence, and thus
> return a NIL sequence list which is handled in that function already. Using
> these two functions will avoid the clutter as well as segfault. If that's
> acceptable, I will provide a patch.

Yeah, you could do that with these two routines as well.  The result
would be the same in terms of runtime validity checks.

On Fri, May 24, 2024 at 12:16 PM Michael Paquier <michael@paquier.xyz> wrote:

On Fri, May 24, 2024 at 11:58:51AM +0530, Ashutosh Bapat wrote:
> If we are looking for avoiding a segfault and get a message which helps
> debugging, using get_attname and get_attnum might be better options.
> get_attname throws an error. get_attnum doesn't throw an error and returns
> InvalidAttnum which won't return any valid identity sequence, and thus
> return a NIL sequence list which is handled in that function already. Using
> these two functions will avoid the clutter as well as segfault. If that's
> acceptable, I will provide a patch.

Yeah, you could do that with these two routines as well.  The result
would be the same in terms of runtime validity checks.

PFA patch using those two routines.