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

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.

> As I mentioned earlier, I haven't found a case where this actually causes
> a crash, so Etsuro-san's analysis seems valid. That said, I still think
> it's safer to guard against NULL by checking fpextra before accessing
> its fields, as is done elsewhere.

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.

Best regards,
Etsuro Fujita