Re: Avoid possible dereference null pointer (contrib/postgres_fdw/postgres_fdw.c) - Mailing list pgsql-hackers

On Wed, Jun 18, 2025 at 8:33 PM Ranier Vilela <ranier.vf@gmail.com> wrote:
> Em qua., 18 de jun. de 2025 às 07:29, Etsuro Fujita <etsuro.fujita@gmail.com> escreveu:
>> On Tue, Jun 17, 2025 at 11:03 PM Fujii Masao
>> <masao.fujii@oss.nttdata.com> wrote:
>> > On 2025/06/17 20:37, Ranier Vilela wrote:
>> > > Em ter., 17 de jun. de 2025 às 06:09, Etsuro Fujita <etsuro.fujita@gmail.com <mailto:etsuro.fujita@gmail.com>>
escreveu:
>> > >     On Tue, Jun 17, 2025 at 2:38 PM Fujii Masao <masao.fujii@oss.nttdata.com
<mailto:masao.fujii@oss.nttdata.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