Avoid possible deference NULL pointer (src/backend/optimizer/path/allpaths.c)
Hackorum builds and tests every patch posted to the lists, not only commitfest submissions. This is Hackorum's own CI rather than the PostgreSQL project's, and it is still under testing - please report anything that looks wrong.
You can run a PostgreSQL built from this patch straight from Docker, with no checkout and no build:
docker run --rm -p 5432:5432 ghcr.io/hackorum-dev/postgres-patch:t50851psql -h localhost -U postgresBuilt from patchset v1 (message #1), September 20, 2026 at 09:55 AM.
Every patchset is also pushed to a branch of our PostgreSQL fork, so you can check out the same tree CI built. Without a PostgreSQL checkout:
git clone --branch t50851_1 https://github.com/hackorum-dev/postgres.gitIn a checkout you already have, add the fork once:
git remote add hackorum https://github.com/hackorum-dev/postgres.gitthen, for this patchset and every later one:
git fetch hackorum t50851_1 && git checkout t50851_1Patchset v1 (message #1) is on t50851_1
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
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