Обсуждение: [PATCH] Fix improper tuple deallocation in import_pg_statist()
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);
Patch:
======
---
src/backend/statistics/extended_stats_funcs.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/src/backend/statistics/extended_stats_funcs.c b/src/backend/statistics/extended_stats_funcs.c
index 0ec77a6..9279904 100644
--- a/src/backend/statistics/extended_stats_funcs.c
+++ b/src/backend/statistics/extended_stats_funcs.c
@@ -1509,7 +1509,7 @@ import_pg_statistic(Relation pgsd, JsonbContainer *cont,
pgstup = heap_form_tuple(RelationGetDescr(pgsd), values, nulls);
pgstdat = heap_copy_tuple_as_datum(pgstup, RelationGetDescr(pgsd));
- pfree(pgstup);
+ heap_freetuple(pgstup);
*pg_statistic_ok = true;
--
Regards,
Yonghao Lee
Вложения
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.
--
Tomas Vondra
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] 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] https://postgr.es/m/CAEoWx2=bL41WWcD-4Fxx-buS2Y2G5=9PjkxZbHeFMR6Uy2WNvw@mail.gmail.com
Best regards,
Yonghao Lee
On Thu, Mar 05, 2026 at 02:01:32AM +0100, Tomas Vondra wrote:
> 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.
Right, what is obviously an LLM analysis forgot the context of how
HeapTuple allocations happen when these are formed.
Saying that, this is new code, that I had the idea to commit like
this, and I don't like fresh inconsistencies. We also use
heap_freetuple() in upsert_pg_statistic_ext_data(). Hence, at the
end, done. It was harmless as-is, still..
--
Michael