Re: Avoid possible dereference null pointer (src/backend/catalog/pg_depend.c)

Em qui., 23 de mai. de 2024 às 06:27, Ashutosh Bapat <
ashutosh(dot)bapat(dot)oss(at)gmail(dot)com> escreveu:

>
>
> On Thu, May 23, 2024 at 5:52 AM Michael Paquier <michael(at)paquier(dot)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().
>
IMO, Assert there is no better solution here.

> 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.
>
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.

So, v3, implements it this way.

best regards,
Ranier Vilela