Re: [PATCH] Fix improper tuple deallocation in import_pg_statist()

On 3/5/26 02:01, Tomas Vondra<tomas@vondra.me> wrote:

On 3/5/26 01:40, yonghao_lee wrote:

Hi hackers,

I found a tuple deallocation issue in the import_pg_statistic() function
in src/backend/statistics/extended_stats_funcs.c. The code uses pfree()
to release a
HeapTuple, which doesn't properly free the underlying tuple data.

Bug Description:
================

In import_pg_statistic(), after heap_form_tuple() creates a HeapTuple and
heap_copy_tuple_as_datum() copies it to a Datum, the code attempts to free
the temporary HeapTuple using pfree():

pgstup = heap_form_tuple(RelationGetDescr(pgsd), values, nulls);
pgstdat = heap_copy_tuple_as_datum(pgstup, RelationGetDescr(pgsd));
pfree(pgstup); /* <-- BUG: Improper tuple release */

The Problem:
============

HeapTuple is a pointer to HeapTupleData structure, which contains a nested
pointer t_data pointing to the actual tuple header and data:

typedef struct HeapTupleData {
uint32 t_len;
ItemPointerData t_self;
Oid t_tableOid;
HeapTupleHeader t_data;
} HeapTupleData;

Using pfree(pgstup) only frees the HeapTupleData structure itself
, but leaves the underlying tuple data unfreed.

The Fix:
========

Replace pfree(pgstup) with heap_freetuple(pgstup), which properly frees both
the HeapTupleData structure and the underlying t_data:

-    pfree(pgstup);
+    heap_freetuple(pgstup);

Except that heap_freetuple is defined like this:

void
heap_freetuple(HeapTuple htup)
{
pfree(htup);
}

so this does not change anything. This report seems to miss how tuples
are allocated in heap_form_tuple.

Hi Tomas,

I acknowledge that pfree() works correctly here given the current implementation.

However, for API consistency (heap_form_tuple -> heap_freetuple) against future changes,
I maintain that heap_freetuple() is the better choice. If some new code is added to heap_freetuple
in future, then the mismatch would lead to a bug silently.

I saw there was a similar discussion [1]/messages/by-id/CAEoWx2=bL41WWcD-4Fxx-buS2Y2G5=9PjkxZbHeFMR6Uy2WNvw@mail.gmail.com where the patch fixed a mismatched index_open/relation_close,
in that case, though lockmode is NoLock,thus index_close behaves the same as relation_close,
but the patch commit 171198ff2a57fafe0772ec9d3dfbf061cffffacd message says: "This was harmless
because index_close() and relation_close() do the exact same work, still inconsistent with the rest of
the code tree as routines opening and closing a relation based on a relkind are expected to match,
at least in name."

Looking forward to your guidance.

[1]: /messages/by-id/CAEoWx2=bL41WWcD-4Fxx-buS2Y2G5=9PjkxZbHeFMR6Uy2WNvw@mail.gmail.com

Best regards,
Yonghao Lee