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

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

Hi,

On Tue, Jun 17, 2025 at 2:38 PM Fujii Masao <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?

Best regards,
Etsuro Fujita

Em ter., 17 de jun. de 2025 às 06:09, Etsuro Fujita <etsuro.fujita@gmail.com>
escreveu:

Hi,

On Tue, Jun 17, 2025 at 2:38 PM Fujii Masao <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.

/*
* Get cost/size estimates with help of remote server. Save the
* values in fpinfo so we don't need to do it again to generate the
* basic foreign path.
*/
estimate_path_cost_size(root, baserel, NIL, NIL, NULL,
&fpinfo->rows, &fpinfo->width,
&fpinfo->disabled_nodes,
&fpinfo->startup_cost, &fpinfo->total_cost);

best regards,
Ranier Vilela

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

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

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.

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.

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.

best regards,
Ranier Vilela

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

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