Skip to content

fix: double free if a plan is cached and rerun - #645

Open
imor wants to merge 22 commits into
mainfrom
rs/fix-double-free
Open

fix: double free if a plan is cached and rerun#645
imor wants to merge 22 commits into
mainfrom
rs/fix-double-free

Conversation

@imor

@imor imor commented Sep 3, 2026

Copy link
Copy Markdown
Contributor

This PR fixes a double free/use after free bug in the core wrappers framework logic. The bug occurred due to an assumption about how Postgres runs the plan and scan phases of a query. Postgres can cache a query once and then execute it repeatedly without ever planning it again. The code in wrappers assumed that Postgres will never cache a plan. This assumption lead to wrappers creating the FdwState object at the beginning of the plan phase, and freeing it unconditionally at the end of the first scan phase. If the plan was cached, the new scan phase will then use a freed object and free it again at the end of that phase, and so on.

Since Postgres can cache a plan, it makes a deep copy of the plan via a call to copyObject before starting the scan phase. To fix the bug we now ensure that any object created during the plan phase are dropped at the end of that phase and any state we need to carry over from the plan phase to the scan phase we serialize into a List via the fdw_private member, and reconstitute an FdwState struct from this deserialized state. This struct is then dropped at the end of the scan phase. This decoupling of lifetimes of objects in plan vs scan phases is what fixes the bug.

A large portion of the code is this new serialization/deserialization logic, but there were also some other side effects of fixing this bug. Specifically with the above fix we initially started creating two instances of FdwState struct (and the Fdw instance) which would break the assumption of FDWs that their instance would only be created ever once, leading to some potential bugs. Less severely, it could also affect performance if and FDW instance performed significant work in their new() method, like establishing connections etc. To fix this we also ensure that we now only create an FDW instance during the scan phase. This does mean we had to remove a &self argument from certain plan phase functions, but the good news is that this change did not break any of the FDWs.

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🔵 Needs a closer look

It makes substantial unsafe/memory-context and plan-lifecycle changes in core scan serialization/execution paths that warrant careful human validation beyond automated review.

Pull request overview

This PR fixes a backend crash/double-free scenario when Postgres caches a foreign scan plan and re-executes it (e.g., via PREPARE/EXECUTE). It does so by ensuring the data passed through fdw_private is safely deep-copyable across planning/execution boundaries and by preventing FDW instances from being constructed during planning.

Changes:

  • Reworked scan planning/execution state handoff: serialize planning-time scan inputs into a Postgres List of Const nodes (safe for copyObject) and reconstruct a fresh FdwState per execution.
  • Updated the ForeignDataWrapper trait planning hooks to be instance-free (get_rel_size, supported_aggregates, supports_group_by) and adjusted native FDWs + docs accordingly.
  • Added a regression test to validate cached-plan re-execution does not crash and that planning/execution hooks run the expected number of times.
File summaries
File Description
wrappers/src/supabase_wrappers_tests.rs Adds a regression test covering prepared/cached plan re-execution lifecycle behavior.
wrappers/src/fdw/mysql_fdw/mysql_fdw.rs Updates aggregate-pushdown hooks to match new static trait method signatures.
wrappers/src/fdw/mssql_fdw/mssql_fdw.rs Updates aggregate-pushdown hooks to match new static trait method signatures.
wrappers/src/fdw/clickhouse_fdw/clickhouse_fdw.rs Updates aggregate-pushdown hooks to match new static trait method signatures.
wrappers/src/fdw/bigquery_fdw/bigquery_fdw.rs Removes instance-based get_rel_size impl and updates aggregate-pushdown hooks to static signatures.
supabase-wrappers/src/utils.rs Removes the old SerdeList helper now that scan state is serialized differently.
supabase-wrappers/src/upper.rs Uses static planning-time hooks for aggregate support checks (no FDW instance required).
supabase-wrappers/src/scan.rs Core fix: makes fdw_private deep-copyable, rebuilds scan state per execution, and avoids planning-time instance creation.
supabase-wrappers/src/qual.rs Tracks original Const nodes for quals so their values can survive plan caching/copying correctly.
supabase-wrappers/src/interface.rs Extends Qual with value_const and updates trait docs/signatures for planning-time hooks.
docs/guides/query-pushdown.md Updates documentation examples for the new static aggregate hook signatures.
CLAUDE.md Updates internal repo guide snippets to reflect the new trait hook signatures and semantics.
Review details
  • Files reviewed: 12/12 changed files
  • Comments generated: 6
  • Review effort level: Lite

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

Comment thread supabase-wrappers/src/scan.rs
Comment thread wrappers/src/supabase_wrappers_tests.rs
Comment thread supabase-wrappers/src/interface.rs Outdated
Comment thread supabase-wrappers/src/scan.rs Outdated
Comment thread supabase-wrappers/src/scan.rs Outdated
Comment thread supabase-wrappers/src/scan.rs Outdated
imor and others added 8 commits September 3, 2026 19:52
Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
This reverts commit 422b86d.

Copilot suggested malformed fix for little gain.
@imor
imor force-pushed the rs/fix-double-free branch from 5502ae0 to 4467d30 Compare September 3, 2026 14:47
@imor
imor marked this pull request as ready for review September 3, 2026 14:57
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants