Re: InvalidateConstraintCacheCallBack() can free fpmeta while it's in use

On Thu, Aug 6, 2026 at 11:50 PM Amit Langote <amitlangote09(at)gmail(dot)com> wrote:
>
> Hi,
>
> $SUBJECT was reported to me off-list.
>
> Commit e484b0eea6 added a pfree() of the entry's FastPathMeta to
> InvalidateConstraintCacheCallBack(), to fix a leak I'd noticed under
> CLOBBER_CACHE_ALWAYS: ri_LoadConstraintInfo() NULLs riinfo->fpmeta
> when it refills a recycled entry, so the old allocation was lost. That
> was the wrong place to fix it. The comment atop the callback says:
>
> * Note: at the time a cache invalidation message is processed there may be
> * active references to the cache. Because of this we never remove entries
> * from the cache, but only mark them invalid, which is harmless to active
> * uses.
>
> The fast-path metadata is subject to the same rule, and I didn't take
> sufficient notice of it. ri_FastPathCheck() and
> ri_FastPathBatchFlush() latch riinfo->fpmeta into a local, and
> ri_FastPathFlushArray() also takes FmgrInfo pointers into it
> (cast_func_finfo, eq_opr_finfo) before its "walk all matches" loop.
> That loop calls index_getnext_slot(), ri_LockPKTuple(), and, for a
> cross-type FK, the user's cast and equality functions. Any of those
> can accept invalidation messages, so the callback can free the object
> the loop is still reading, after which the loop calls through
> FmgrInfos in freed memory. This doesn't require a concurrent DDL to
> reach: a user-defined cast or equality function that performs DDL
> itself will process the invalidation synchronously in the middle of
> the loop.
>
> To be clear on scope, the issue is the object's lifetime, not the
> freshness of its contents. Stale contents are covered by the
> parenthetical above: under the locks the flush already holds, nothing
> that the metadata derives from can change underneath it, so an
> invalidation observed mid-loop would rebuild identical metadata.
>
> So I propose to unlink the metadata from the entry in the callback,
> leaving the next check to rebuild it as it does today, and defer the
> free to a point where no RI check can be on the stack -- AtEOXact_RI()
> is the obvious candidate. Detached objects get chained and released
> there, and the call sites that test riinfo->fpmeta == NULL don't
> change. The cost is holding the memory until the end of the
> transaction rather than freeing it immediately. This cost is bounded
> by a few kB per detached object and reached only by transactions that
> interleave DDL with FK-checking DML.
>
> Other approaches I looked at and am not proposing: keeping the
> allocation for the entry's lifetime and repopulating it in place,
> which would be a smaller patch but overwrites the FmgrInfos while a
> call through one of them may still be in progress, and
> fmgr_info_copy() clears fn_extra, so any per-call-site state the
> running function cached there disappears underneath it; having the
> flush take a private copy up front, which is easy to reason about but
> costs a few kB of memcpy per flush; and refcounting the cache entry so
> it's freed when the last reference drops, which is what plancache does
> for the analogous problem and is probably the better long-term answer,
> but is more machinery than I'd want to introduce at this point in the
> cycle.
>
> I'll post a patch shortly and add an open item once this hits the archive.

Patches attached.

0001 is what I described upthread. InvalidateConstraintCacheCallBack()
unlinks the metadata from the cache entry rather than freeing it, so
the next check rebuilds it exactly as it does today, and the detached
object is chained onto a list that AtEOXact_RI() releases, at which
point no RI check can be on the stack. The call sites that test
riinfo->fpmeta == NULL are unchanged.

0002 is what was 0004 in the series I last posted at [1], and fixes a
separate leak in the same struct. ri_populate_fastpath_metadata()
copies the cast and equality FmgrInfos with fn_mcxt set to
TopMemoryContext, and fn_mcxt is scratch space for the called
function: record_eq(), and the record I/O functions generally,
allocate their per-call cache there and keep a pointer to it in
fn_extra. That scratch space outlives the metadata, so any cached data
is orphaned each time the metadata is discarded. 0002 gives the
FmgrInfos a context of their own and deletes it alongside the struct
in the AtEOXact_RI() cleanup.

0002 must follow 0001 because there's no safe place to delete the
context until 0001 adds the deferred release. In the callback it would
be a second use-after-free, this time in memory the called function
owns rather than memory we do: an in-flight record_eq() has fn_extra
pointing into the context and fn_mcxt at the context itself, so it
would read back freed state or palloc into a deleted context.

Ayush Tiwari reviewed 0002 at [1], and while doing so, independently
raised the same lifetime problem this thread is about and 0001 is
meant to fix.

I've added open items for both, noting that the second must be
committed with / after the first.

[1] https://www.postgresql.org/message-id/CA%2BHiwqHHK2D69%2BQqAom%2Bth1kjZGK-0dYnrZujkEGFWpX_ZtxqQ%40mail.gmail.com

--
Thanks, Amit Langote