Re: Avoid possible dereference null pointer (contrib/postgres_fdw/postgres_fdw.c)

On Wed, Jun 18, 2025 at 8:33 PM Ranier Vilela <ranier(dot)vf(at)gmail(dot)com> wrote:
> Em qua., 18 de jun. de 2025 às 07:29, Etsuro Fujita <etsuro(dot)fujita(at)gmail(dot)com> escreveu:
>> On Tue, Jun 17, 2025 at 11:03 PM Fujii Masao
>> <masao(dot)fujii(at)oss(dot)nttdata(dot)com> wrote:
>> > On 2025/06/17 20:37, Ranier Vilela wrote:
>> > > Em ter., 17 de jun. de 2025 às 06:09, Etsuro Fujita <etsuro(dot)fujita(at)gmail(dot)com <mailto:etsuro(dot)fujita(at)gmail(dot)com>> escreveu:
>> > > On Tue, Jun 17, 2025 at 2:38 PM Fujii Masao <masao(dot)fujii(at)oss(dot)nttdata(dot)com <mailto:masao(dot)fujii(at)oss(dot)nttdata(dot)com>> wrote:
>> > > > adjust_foreign_grouping_path_cost(root, pathkeys,
>> > > > retrieved_rows, width,
>> > > > - fpextra->limit_tuples,
>> > > > + fpextra ? fpextra->limit_tuples : 0.0,
>> > > > &disabled_nodes,
>> > > > &startup_cost, &run_cost);
>> > > >
>> > > > I couldn't find a query that would reach this code path with
>> > > > fpextra == NULL, but I agree the current code is fragile.
>> > > > So I think it's a good idea to add the check before accessing
>> > > > the field.
>> > >
>> > > We get here only when called from add_foreign_ordered_paths() or
>> > > add_foreign_final_paths(), in which cases fpextra is always set, so it
>> > > cannot be NULL. No?
>> > >
>> > > False.
>> > >
>> > > In the function *postgresGetForeignRelSize* there is one call,
>> > > where fpextra is NULL.
>> >
>> > I think Etsuro-san meant that the problematic code path is only reachable
>> > when estimate_path_cost_size() is called from add_foreign_ordered_paths() or
>> > add_foreign_final_paths(), and in those cases, fpextra is guaranteed to
>> > be non-NULL. In other cases, such as postgresGetForeignRelSize(),
>> > fpextra can be NULL, but the code path in question isn't reached - for example,
>> > because pathkeys is NIL.
>>
>> Right. Another thing you need to be careful about here is the input
>> relation’s RelOptKind. As asserted right above
>> adjust_foreign_grouping_path_cost() in the code path in question, the
>> RelOptKind to exercise the code path to is limited to
>> RELOPT_UPPER_REL. Since 1) we only call estimate_path_cost_size() to
>> RELOPT_UPPER_REL in add_foreign_grouping_paths(),
>> add_foreign_ordered_paths(), and add_foreign_final_paths(), and 2) we
>> call it without pathways in the first function and with pathways in
>> the latter two functions, the code path can only be exercised when we
>> call it from the latter two functions, in which cases, as you
>> mentioned, we always set fpextra in those functions before calling it,
>> so fpextra cannot be NULL.
>
> Ok. I will not insist on this point.
> But I think this is unnecessary mental gymnastics.
> From a maintainability point of view, it is harmful.
> It would be enough to protect the pointer, as is already done in the rest of the code,
> and future uses of this function would be much simpler.

I sympathize with you, but note that fpextra cannot be NULL (whereas
it can be NULL in the rest of the code), so no need to do the check
you proposed; doing so is just a waste of cycles. I think it would be
good to do so if and when we really need to.

>> Considering fpextra cannot be NULL, I think the proposed change is
>> something more than necessary. IMO I think it is more appropriate to
>> just add an assertion and a comment for that like the attached, to
>> avoid this kind of confusion. I think I should have done so when
>> committing this.
>
> I disapprove of this change, for me it worsens readability.
> It is better to continue without any changes, then.
> But if there is consensus, go ahead.

I'm not sure this worsens readability, but I still think it would be
useful to avoid the same confusion in the future, so barring
objections, I will push the patch as a master-only improvement.

Thanks!

Best regards,
Etsuro Fujita