Thread: Re: Avoid possible dereference null pointer (contrib/postgres_fdw/postgres_fdw.c)
Re: Avoid possible dereference null pointer (contrib/postgres_fdw/postgres_fdw.c)
From
Fujii Masao
Date:
On 2025/06/17 4:48, Ranier Vilela wrote:
> Hi.
>
> In the function *estimate_path_cost_size* the parameter
> fpextra can be NULL.
Yes.
> It is necessary to always check its validity,
> as is already done in other parts of the source.
>
> patch attached.
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.
Regards,
--
Fujii Masao
NTT DATA Japan Corporation
Re: Avoid possible dereference null pointer (contrib/postgres_fdw/postgres_fdw.c)
From
Fujii Masao
Date:
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: > > Hi, > > 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. 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
Re: Avoid possible dereference null pointer (contrib/postgres_fdw/postgres_fdw.c)
From
Etsuro Fujita
Date:
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. > 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