Avoid possible deference NULL pointer (src/backend/optimizer/path/allpaths.c)
Hi.
Per Coverity.
All call sites of function *get_cheapest_path_for_pathkeys* checks
for NULL returns.
So, it is highly likely that the function will return NULL.
IMO, the Assert in this particular call, is not fully effective.
Fix removing the Assert and always check if the return is NULL.
best regards,
Ranier Vilela
Attachments:
fix-possible-null-dereference-allpatchs.patchapplication/octet-stream; name=fix-possible-null-dereference-allpatchs.patchDownload
diff --git a/src/backend/optimizer/path/allpaths.c b/src/backend/optimizer/path/allpaths.c
index 3364589391..89c273e6cd 100644
--- a/src/backend/optimizer/path/allpaths.c
+++ b/src/backend/optimizer/path/allpaths.c
@@ -2024,8 +2024,8 @@ get_cheapest_parameterized_child_path(PlannerInfo *root, RelOptInfo *rel,
required_outer,
TOTAL_COST,
false);
- Assert(cheapest != NULL);
- if (bms_equal(PATH_REQ_OUTER(cheapest), required_outer))
+
+ if (cheapest != NULL && bms_equal(PATH_REQ_OUTER(cheapest), required_outer))
return cheapest;
/*
On 05.01.2025 02:29, Ranier Vilela wrote:
Hi.
Per Coverity.
All call sites of function *get_cheapest_path_for_pathkeys* checks
for NULL returns.So, it is highly likely that the function will return NULL.
IMO, the Assert in this particular call, is not fully effective.
Fix removing the Assert and always check if the return is NULL.
best regards,
Ranier Vilela
Hi!
Thanks for noticing this. If this happens in the planner, it poses a
serious risk of a segmentation fault that could crash the instance if a
NULL pointer is dereferenced. Since checking for NULL is very cheap, I
support this patch.
--
Best regards,
Ilia Evdokimov,
Tantor Labs LLC.
Hi.
Em qua., 5 de fev. de 2025 às 13:51, Ilia Evdokimov <
ilya.evdokimov@tantorlabs.com> escreveu:
On 05.01.2025 02:29, Ranier Vilela wrote:
Hi.
Per Coverity.
All call sites of function *get_cheapest_path_for_pathkeys* checks
for NULL returns.So, it is highly likely that the function will return NULL.
IMO, the Assert in this particular call, is not fully effective.
Fix removing the Assert and always check if the return is NULL.
best regards,
Ranier VilelaHi!
Thanks for noticing this. If this happens in the planner, it poses a
serious risk of a segmentation fault that could crash the instance if a
NULL pointer is dereferenced. Since checking for NULL is very cheap, I
support this patch.
Thanks for taking a look.
best regards,
Ranier Vilela
On 5 Jan 2025, at 00:29, Ranier Vilela <ranier.vf@gmail.com> wrote:
Hi.
Per Coverity.
All call sites of function *get_cheapest_path_for_pathkeys* checks
for NULL returns.So, it is highly likely that the function will return NULL.
IMO, the Assert in this particular call, is not fully effective.
Fix removing the Assert and always check if the return is NULL.
Yet the author wrote an Assert here (over a decade ago), so rather than blindly
changing that it seems reasonable to motivate a patch like this with an
investigation on what the Assert means here. The fact that Coverity complains
is far from conclusive evidence that something is wrong.
--
Daniel Gustafsson
But what should we do if cheapest == NULL further? Should we return NULL
of get_cheapest_parameterized_child_path() function?
If it is, we should write it like this:
if (cheapset == NULL || bms(PATH_REQ_OUTER(cheapset), required_outer))
return cheapest;
I'll look into this issue further.
--
Best regards,
Ilia Evdokimov,
Tantor Labs LLC.
Em qua., 5 de fev. de 2025 às 14:09, Ilia Evdokimov <
ilya.evdokimov@tantorlabs.com> escreveu:
But what should we do if cheapest == NULL further? Should we return NULL
of get_cheapest_parameterized_child_path() function?If it is, we should write it like this:
if (cheapset == NULL || bms(PATH_REQ_OUTER(cheapset), required_outer))
return cheapest;
I think no in this case.
If cheapset is NULL, the logic is to continue the find.
What cannot happen is passing a null pointer to bms(PATH_REQ_OUTER.
best regards,
Ranier Vilela
Hi.
Em qua., 5 de fev. de 2025 às 14:08, Daniel Gustafsson <daniel@yesql.se>
escreveu:
On 5 Jan 2025, at 00:29, Ranier Vilela <ranier.vf@gmail.com> wrote:
Hi.
Per Coverity.
All call sites of function *get_cheapest_path_for_pathkeys* checks
for NULL returns.So, it is highly likely that the function will return NULL.
IMO, the Assert in this particular call, is not fully effective.
Fix removing the Assert and always check if the return is NULL.
Yet the author wrote an Assert here (over a decade ago), so rather than
blindly
changing that it seems reasonable to motivate a patch like this with an
investigation on what the Assert means here. The fact that Coverity
complains
is far from conclusive evidence that something is wrong.
This is evidence that we do not have reports about this bug.
In any case, I think it's very unsafe for the future to trust that a
function that returns NULL
will never return in this particular case, don't you think?
best regards,
Ranier Vilela
On 5 Feb 2025, at 18:34, Ranier Vilela <ranier.vf@gmail.com> wrote:
Em qua., 5 de fev. de 2025 às 14:08, Daniel Gustafsson <daniel@yesql.se <mailto:daniel@yesql.se>> escreveu:
Yet the author wrote an Assert here (over a decade ago), so rather than blindly
changing that it seems reasonable to motivate a patch like this with an
investigation on what the Assert means here. The fact that Coverity complains
is far from conclusive evidence that something is wrong.This is evidence that we do not have reports about this bug.
Before that can be stated it needs to be determined if this is a bug, this
thread has not done that yet.
--
Daniel Gustafsson
Daniel Gustafsson <daniel@yesql.se> writes:
On 5 Feb 2025, at 18:34, Ranier Vilela <ranier.vf@gmail.com> wrote:
This is evidence that we do not have reports about this bug.
Before that can be stated it needs to be determined if this is a bug, this
thread has not done that yet.
It's not a bug. Since the call specifies NIL pathkeys (meaning it
doesn't care about sort order) and does not insist on a parallel-safe
path, there should always be a path that satisfies it. The only
way it could fail to find a path is if the rel's pathlist is entirely
empty, a case already rejected by the sole caller.
Moreover, the argument that we might not have gotten a report is not
credible. In a production build without an Assert, the next line
would still dump core just as effectively if the result were NULL.
While the proposed patch doesn't break anything, it's removing a
logic cross-check that was put there intentionally. So I don't
find it to be an improvement.
regards, tom lane
On 05.02.2025 21:56, Tom Lane wrote:
It's not a bug. Since the call specifies NIL pathkeys (meaning it
doesn't care about sort order) and does not insist on a parallel-safe
path, there should always be a path that satisfies it. The only
way it could fail to find a path is if the rel's pathlist is entirely
empty, a case already rejected by the sole caller.Moreover, the argument that we might not have gotten a report is not
credible. In a production build without an Assert, the next line
would still dump core just as effectively if the result were NULL.While the proposed patch doesn't break anything, it's removing a
logic cross-check that was put there intentionally. So I don't
find it to be an improvement.regards, tom lane
Ah, if this Assert was intentionally added to ensure that a path must be
always found under the given conditions, and that any issues with this
can be detected in the right place, then I agree. The patch likely makes
worse.
--
Best regards,
Ilia Evdokimov,
Tantor Labs LLC.
Em qua., 5 de fev. de 2025 às 15:56, Tom Lane <tgl@sss.pgh.pa.us> escreveu:
So I don't
find it to be an improvement.
Ok, I'm withdrawing this patch.
Thanks to everyone who contributed to the thread.
best regards,
Ranier Vilela