CF6967 Review: Add malloc attribute to memory allocation functions

security-review · generated from canonical Markdown

Type
security-review

Source
postgresql-master

Slug
commitfest-6967-review

Tags

Updated
2026-08-03T09:10:23.709743+00:00

Verdict / SummaryCommitFest 6967 review of "Add malloc attribute to memory allocation functions" (Tristan Partin). Header-only annotation patch (3 .h files, 0 .c files). Verdict: waiting-on-author. 1 BLOCKER (thread consensus opposes backend annotations), 2 MAJOR (deallocator-attribute semantic mismatch, GCC < 11 guard). All findings verified. Security review: no regressions. Build: clean on GCC 15.2.0, 245/245 tests pass. Coverage: not-applicable (header-only). Repair: frontend-only annotations + GCC >= 11 guard.

Front matter

schema: review-report-v2
target: CF6967 - Add malloc attribute to memory allocation functions
head: 7cb9060dcde08ca63df1f0d701b0674e0e8ae811
diff_sha256: 067e961369b52100ecf6bf8101a3a7e2b6c5426ff6662d21b02f330fb91f363b
verdict: waiting-on-author
blocker_split:
  verified: 1
  unverified: 0

CF6967 Review — Add malloc attribute to memory allocation functions

Patch: "Add malloc attribute to memory allocation functions" (Tristan Partin) CommitFest: 61, Patch 6967 Canonical Message-ID: DJNFBUWD276K.2OPIYJQS75W84@partin.io Thread: pgsql-hackers thread 650381 (2026-07-01 to 2026-07-07, 11 messages) Target: uncommitted working-tree diff (read-only worktree) HEAD/base: 7cb9060dcde08ca63df1f0d701b0674e0e8ae811 Diff SHA (sha256): 067e961369b52100ecf6bf8101a3a7e2b6c5426ff6662d21b02f330fb91f363b Changed files (3): src/include/c.h (+12/-0), src/include/common/fe_memutils.h (+18/-18), src/include/utils/palloc.h (+18/-18)

Verdict

PostgreSQL verdict: waiting-on-author

Blocker split: verified=1, unverified=0

The patch has a verified BLOCKER: the pgsql-hackers thread consensus (led by senior committer Tom Lane) opposes annotating backend allocators because GCC's -analyzer cannot understand memory-context-based lifecycles, producing massive false-positive leak warnings (62→598). The author acknowledges the backend portion "may not be committed." The patch as submitted annotates both frontend AND backend allocators, contradicting this consensus.

The author should split the patch: submit the frontend-only annotations (fe_memutils.h) as the primary patch, and either drop the backend annotations (palloc.h) or submit them separately as exploratory. Additionally, fix the GCC version guard to require __GNUC__ >= 11 for the deallocator-argument form.

---

Findings

[BLOCKER] — Thread consensus opposes the backend portion

Class: process Location: src/include/utils/palloc.h Mechanism: thread-consensus-conflict

Description: The patch annotates both frontend and backend allocators, but the pgsql-hackers thread consensus opposes backend annotations. Tom Lane (senior committer) objects: "gcc has never heard of memory contexts... bogus -Wanalyzer-malloc-leak warnings... annotations that only sort-of match our semantics." The author measured -Wanalyzer-malloc-leak warnings increasing from 62 (master) to 598 (with patch). The thread converges on frontend-only/targeted annotations. The author agrees: "the path forward is two-fold: 1. targeted frontend annotations 2. backend annotations knowing that the patch may not be committed."

Verification: verified (inspection) Evidence: Thread 650381, messages from tgl@sss.pgh.pa.us (2026-07-06, 2026-07-07); author acknowledgment in DJSQVBYR1O3S.W4ZMXLOEPRBH@partin.io; measured warning count 62→598.

Repair:

Instruction: Revert src/include/utils/palloc.h to HEAD (remove all backend allocator annotations). Keep frontend annotations in fe_memutils.h.
Code example (exact-postimage): extern char *pg_strdup(const char *in) pg_attribute_malloc(pg_free); (frontend annotation preserved)
Patch: Unified diff, SHA-256 5caef3148dc00dada1630eb9b2cd008b702405b8313b82e2c9db375f40342420, applies to HEAD 7cb9060dcde08ca63df1f0d701b0674e0e8ae811. The repair patch adds the fixed pg_attribute_malloc macro in c.h (with __GNUC__ >= 11 guard) and frontend annotations in fe_memutils.h, but does NOT modify palloc.h.
Validation: passed (apply-check, diff-check, compile, static-check). Full build on GCC 15.2.0 succeeded with 0 warnings, 0 errors. make check: 245/245 tests passed.
Test example: Inspection: grep -c pg_attribute_malloc src/include/utils/palloc.h returns 0 after repair (17 before).

[MAJOR] — Deallocator-attribute semantic mismatch on backend allocators

Class: correctness Location: src/include/utils/palloc.h:79-82,93-95,157-164 Mechanism: deallocator-attribute-semantic-mismatch

Description: pg_attribute_malloc(pfree) on backend allocators asserts a deallocator contract that most call sites do not honour. Backend memory is typically freed by MemoryContextReset/MemoryContextDelete (bulk context teardown), not individual pfree() calls. GCC cannot see context resets as freeing the chunk, producing 536 new false-positive -Wanalyzer-malloc-leak warnings (62→598).

Verification: verified (inspection) Evidence: src/include/utils/palloc.h lines 79-82, 93-95, 157-164 show pg_attribute_malloc(pfree) on backend allocators. src/backend/utils/mmgr/aset.c:546: AllocSetReset frees all chunks without pfree. src/include/utils/memutils.h:74-78: MemoryContextReset/Delete are primary deallocation mechanisms. Thread evidence: author measured 62→598 warnings.

Repair: Same as F1 — revert palloc.h to HEAD. The false-positive deallocator contract is eliminated.

Validation: passed (apply-check, diff-check, compile, static-check).

[MAJOR] — GCC < 11 guard too broad for deallocator-argument form

Class: correctness Location: src/include/c.h:283-287 Mechanism: compiler-version-guard-too-broad

Description: __has_attribute(malloc) returns true on GCC 5-10, but the deallocator-argument form malloc(dealloc) was only added in GCC 11. On older GCC, pg_attribute_malloc(pfree) expands to __attribute__((malloc(pfree))) which is silently ignored with a -Wattributes warning. With -Werror, this would be a build failure.

Verification: verified (inspection) Evidence: src/include/c.h:283: #if __has_attribute(malloc) && !defined(__clang__). GCC manual: malloc(deallocator) form added in GCC 11. __has_attribute(malloc) returns true on GCC 5+.

Repair:

Instruction: Change guard to #if __has_attribute(malloc) && !defined(__clang__) && __GNUC__ >= 11. Update comment to document GCC 11 requirement.
Code example (exact-postimage): #if __has_attribute(malloc) && !defined(__clang__) && __GNUC__ >= 11
Patch: Included in the combined repair patch (SHA-256 5caef3148dc00dada1630eb9b2cd008b702405b8313b82e2c9db375f40342420).
Validation: passed (apply-check, diff-check, compile, static-check). All affected targets compiled successfully on GCC 15.2.0.
Test example: Inspection: grep '__GNUC__ >= 11' src/include/c.h confirms guard is present.

---

Verification Summary

#	Finding	Severity	Status	Method	Repair Validation
F1	Thread consensus opposes backend portion	BLOCKER	verified	inspection	passed
F2	Deallocator-attribute semantic mismatch	MAJOR	verified	inspection	passed
F3	GCC < 11 guard too broad	MAJOR	verified	inspection	passed

Counts: verified=3, refuted=0, cannot-verify=0. BLOCKER verified=1, unverified=0. MAJOR verified=2, unverified=0.

Repair Validation Summary

A single combined repair patch addresses all three findings:

F1 + F2: Revert src/include/utils/palloc.h to HEAD (remove all backend annotations)
F3: Add __GNUC__ >= 11 to the guard in src/include/c.h
Keep: Frontend annotations in src/include/common/fe_memutils.h (unchanged from original patch)

Repair patch SHA-256: 5caef3148dc00dada1630eb9b2cd008b702405b8313b82e2c9db375f40342420 Applies to HEAD: 7cb9060dcde08ca63df1f0d701b0674e0e8ae811

Validation evidence (executed in disposable scratch):

git apply --check: PASSED
git diff --check: CLEAN
Full build on GCC 15.2.0: EXIT 0, 0 warnings, 0 errors
make check: 245/245 tests passed
cc -Werror=mismatched-dealloc on fe_memutils.c: EXIT 0

---

Changed-Code Coverage

Coverage requirement: not-applicable

Reason: All changed lines are compiler attribute annotations on extern function prototypes in header files (c.h, fe_memutils.h, palloc.h). No .c implementation files changed, no executable code paths, no branches. patch_readiness confirms 0 C symbols changed across 0 C files.

Non-instrumented changes: Header-only annotation patch: 3 .h files, 0 .c files. All changed lines are pg_attribute_malloc macro definition and attribute annotations on extern prototypes. No runtime behavior change.

Runtime validation: Full make check (245/245 tests pass) executed by coordinator in disposable scratch on GCC 15.2.0. This confirms the patch compiles cleanly and passes the regression suite.

---

Security Review

Security verdict: No security regressions.

The security reviewer found 0 BLOCKER, 0 MAJOR, 0 MINOR findings. Key findings:

All deallocator pairings verified correct (pg_free for frontend pg_malloc family, pfree for backend palloc family)
repalloc/pg_realloc correctly NOT annotated (realloc may return original pointer)
MemoryContextAlloc* use bare pg_attribute_malloc() (no deallocator) — safe conservative choice
The !defined(__clang__) exclusion is a safe no-op (Clang doesn't implement deallocator-argument form)
Strictly additive: no existing warnings suppressed because none existed before

---

pgsql-hackers Thread Ledger

Thread: 650381 (pgsql-hackers, 2026-07-01 to 2026-07-07, 11 messages) Status: reconciled Ledger report: commitfest-6967-review-handoff-thread-ledger

Consensus reached: Yes. The thread converged on a two-part path forward:

Frontend/targeted annotations (PGfreemem, frontend allocators) as the viable patch
Backend annotations as exploratory, expected not to be committed

Live objection: Yes. Tom Lane (senior committer) objects to the backend portion as submitted.

Per-finding dispositions:

F1 (BLOCKER): committer-corroborated — Tom Lane explicitly opposes backend annotations
F2 (MAJOR): committer-corroborated — Tom Lane raises semantic mismatch, author confirms with data
F3 (MAJOR): new — not discussed in thread, technical concern identified during review

Key messages:

924283.1783312014@sss.pgh.pa.us (Tom Lane, 2026-07-06): "I'm skeptical... gcc has never heard of memory contexts... bogus -Wanalyzer-malloc-leak warnings"
DJSQVBYR1O3S.W4ZMXLOEPRBH@partin.io (Tristan Partin, 2026-07-07): "the path forward is two-fold: 1. targeted frontend annotations 2. backend annotations knowing that the patch may not be committed"

---

Aggregated Test Plan

Test	Vehicle	Files	Gate
Verify no backend annotations	inspection	src/include/utils/palloc.h	grep -c pg_attribute_malloc == 0
Verify GCC >= 11 guard	inspection	src/include/c.h	Guard includes __GNUC__ >= 11
Frontend mismatched-dealloc	compile	src/common/fe_memutils.c	cc -Werror=mismatched-dealloc exits 0
Full regression	regress	src/test/regress/	make check 245/245 (coordinator verified)

---

Provenance

Model: glm-5.2:cloud (user-fixed) Model routing: Catalog recommend_model returned unavailable (no_host_cap_model_match) for all seats. Falling back to session default per harness instruction. Policy version 1, hash 997c035512fa1b35aa17149ea386a861af531ec05839ce06810f38268a3caebe.

Specialist seats:

Seat	Handoff slug	markdown_sha256
patch-reviewer	commitfest-6967-review-handoff-patch-reviewer	0d9cdd62e5109a313fced0fcaae6b9d6d6ca27f4d1302aace0cd00092e8285d7
security-reviewer	commitfest-6967-review-handoff-security-reviewer	6276cf6bc66d3fb1decb21a3b5b281d75c63c6a5d09d858c5a3e9f4988450491
finding-verifier	commitfest-6967-review-handoff-verifier	9c6e7e0765455e8c0d5fb9d2b237d55ad47ce74a463f22e0c225606c568058e7
thread-reconciliation	commitfest-6967-review-handoff-thread-ledger	864261411665a10b95d84fd7c9f0b1abf5336f20595559a980d4d92e3378f81e

v2 artifact validation: PASSED (exit 0)

---

Residual Risk

The frontend-only portion of the patch (with the GCC >= 11 guard fix) is ready for committer consideration. The backend portion should be removed per thread consensus. The GCC version guard fix (F3) is a correctness improvement that prevents silent annotation loss on GCC 5-10.

Maintainers

src/include/utils/palloc.h: Tom Lane (3), Bruce Momjian (3), Peter Eisentraut (1) — 3-year window
src/include/c.h: Peter Eisentraut (41), Tom Lane (7), Nathan Bossart (5) — 3-year window
src/include/common/fe_memutils.h: Tom Lane (3), Bruce Momjian (3) — 3-year window

CC: Tom Lane (tgl@sss.pgh.pa.us) — senior committer with objections to backend annotations; Peter Eisentraut (peter@eisentraut.org) — suggested frontend-only approach; Michael Paquier (michael@paquier.xyz) — asked about PQfreemem WIN32 use case.