Use-after-free of a shared Path in add_path()/add_partial_path()

Hi,

While investigating a customer-reported planner crash ("ERROR: unrecognized
node type: N"), I found a use-after-free bug in
add_path()/add_partial_path()
that turns out to be a generalization of an issue reported here in 2023,
which
stalled for lack of a self-contained reproducer:

https://www.postgresql.org/message-id/CAM2%2B6%3DUC1mcVtM0Y_LEMBEGHTM58HEkqHPn7vau_V_YfuZjEGg%40mail.gmail.com

This time I have a reproducer (below), and it turns out the same underlying
defect shows up independently in two different places in the planner, so I
think it's worth fixing generally rather than patching one call site.

My teammate Shruthi internally reported the crash that got me looking at
this
again; once I saw it, the 3-year-old thread above came back to me, and I
ended
up fixing both. Thanks to her for the report.

Note up front: the reproducer below (case 1) only crashes on REL_18_STABLE
and earlier. On REL_19_STABLE, add_partial_path()'s dominance check was
rewritten to compare startup cost as well as total cost, and that
incidentally avoids the specific comparison that discards the shared path
for
this data. The underlying bug is still present on REL_19_STABLE; I just
haven't found data that gets the new cost comparison to hit it.

The problem (see follow-up email for the reproducer)
-----------

add_path() and add_partial_path() pfree() a dominated old_path, or a
rejected
new_path, on the assumption documented in add_path()'s own header comment:

"As noted in optimizer/README, deleting a previously-accepted Path
is safe because we know that Paths of this rel cannot yet be
referenced from any other rel, such as a higher-level join."

Two existing callers violate that assumption by handing in a path that is
still a live member of a *different* rel's pathlist/partial_pathlist,
without
copying it:

1. grouping_planner() exposes a scan/join rel's partial paths to the outer
query level's final rel:

foreach(lc, current_rel->partial_pathlist)
{
Path *partial_path = (Path *) lfirst(lc);
add_partial_path(final_rel, partial_path);
}

without removing them from current_rel->partial_pathlist. If
generate_useful_gather_paths() already built a Gather/Gather Merge path
over one of those entries (it doesn't remove the entry from
partial_pathlist either), and that Gather path has since been carried
into
final_rel->pathlist too, a later dominance comparison inside
add_partial_path() can decide the shared partial path is dominated by
some
other promoted candidate and pfree() it -- leaving the Gather/Gather
Merge
path's subpath pointer dangling.

This crashes planning of a UNION branch that joins parallel-safe
relations
and has a set-returning function in its target list.

2. create_ordered_paths() builds the ORDERED upperrel by, for each path in
input_rel->pathlist that's already sorted per root->sort_pathkeys,
passing
that same Path pointer straight through:

if (is_sorted)
sorted_path = input_path;
...
add_path(ordered_rel, sorted_path);

input_path remains a live member of input_rel->pathlist throughout, so
it's
the same hazard. This is the one from the 2023 thread linked above; it's
otherwise inert, since nothing else revisits the superseded input_rel's
pathlist during normal planning, which is why it only ever showed up as
sporadic "WARNING: could not dump unrecognized node type" while debugging
postgres_fdw with an ad hoc elog(INFO, "foreignrel: %s",
nodeToString(foreignrel)) dropped into postgresGetForeignPlan(). It's
still
reproducible today the same way, via contrib/postgres_fdw's own
regression
suite (the existing "subquery+MAX" test, unmodified) with that debug line
temporarily restored.

The fix
-------

Rather than special-case each known sharing pattern, 0001 adds one general
check: every Path freshly built for parent_rel already has path->parent
stamped to parent_rel by its create_*_path() constructor, before
add_path()/add_partial_path() is ever called with that same rel. So a
path->parent != parent_rel mismatch can only mean the path is on loan from
its
true owning rel, and pfree()'ing it here would leave that rel's list with a
dangling entry. Skip the pfree() in that case, alongside the existing
IndexPath exemption (which covers a different, same-rel sharing pattern --
an
IndexPath referenced as a child of a BitmapHeapPath -- that this check
doesn't
and needn't cover).

No API changes: add_path() and add_partial_path() keep their existing
signatures, and no caller needs to change.

0002 adds a regression test for the UNION+parallel+SRF crash (case 1 above)
to
select_parallel.sql. I did not add one for the
create_ordered_paths()/postgres_fdw case, since I couldn't find a way to
make
that corruption observable without adding debug code.

Testing
-------

Developed against REL_18_STABLE (--enable-cassert, --enable-debug build):
full
regression suite passes, confirmed the fix addresses case 2 as
well as case 1).

Given this affects both add_path() and add_partial_path() and, per the 2023
thread, has been present for a while, I'd guess this warrants a back-patch
once a fix is agreed on, but I'll defer to reviewers on how far back makes
sense.

Thanks,