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

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:
>
> Hi,
>
> 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.

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.

Regards,

--
Fujii Masao
NTT DATA Japan Corporation