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
Attachment
Re: Avoid possible dereference null pointer (contrib/postgres_fdw/postgres_fdw.c)
From
Etsuro Fujita
Date:
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
Re: Avoid possible dereference null pointer (contrib/postgres_fdw/postgres_fdw.c)
From
Etsuro Fujita
Date:
On Fri, Jul 4, 2025 at 2:41 PM Etsuro Fujita <etsuro.fujita@gmail.com> wrote: > 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: > >> 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. Pushed after modifying the comment a little bit and fixing indentation. Best regards, Etsuro Fujita