schema: review-report-v2
target: CommitFest 7261: PQpassfileLookup libpq passfile exposure
base: 0c5d6269614e107d1d2d669f82f63f7e232b30c9
head: 0c5d6269614e107d1d2d669f82f63f7e232b30c9
verdict: ready-for-committer
blocker_counts: verified=0 unverified=0
scope_identity: 4d4ae07c2c9a409f50caf5a7fa753ed887c47435b14f447dd007e76e29c33fb3
model: glm-5.2:cloud
source: postgresql-master
slug: commitfest-7261-review

CommitFest 7261 — PQpassfileLookup() libpq passfile exposure

• Source: postgresql-master
• Slug: commitfest-7261-review
• Verdict: ready-for-committer
• Blockers: 0 verified, 0 unverified
• Scope identity (diff SHA256): 4d4ae07c2c9a409f50caf5a7fa753ed887c47435b14f447dd007e76e29c33fb3
• Base/HEAD: 0c5d6269614e107d1d2d669f82f63f7e232b30c9 (merge-base == HEAD; staged-only diff)
• Model: glm-5.2:cloud (user-fixed)
• Date: 2026-09-14

Patch summary

Adds a new public libpq API PQpassfileLookup(hostname, port, dbname, username, passfile) exposing the existing static passwordFromFile() pgpass lookup so applications connecting through an intermediary (e.g. local SSH tunnel) can look up the password under the real server's host/port and then feed it to PQconnectdb as the password parameter. NULL/empty hostname→localhost (incl. default socket dir), NULL/empty port→DEF_PGPORT_STR, NULL/empty dbname or username→no password. NULL/empty passfile→PGPASSFILE env or the default .pgpass file location. Returns malloc'd string (free with PQfreemem) or NULL. Adds LIBPQ_HAS_PASSFILE_LOOKUP feature macro, export PQpassfileLookup 212, SGML docs, --passfile mode in libpq_testclient, and TAP test t/007_passfile.pl (13 subtests).

Findings summary

Blockers: 0. Majors: 0. Minors: 3 (F-001, S2, COV-1). Nits: 3 (F-002, F-003, S3-refuted).

Verification Summary

All 6 findings were verified against source by the pg-finding-verifier seat in disposable worktrees (HEAD 0c5d6269614e107d1d2d669f82f63f7e232b30c9, scope identity 4d4ae07c...). Validation methods: apply-check, diff-check, compile (meson+ninja), targeted-test (meson test libpq/007_passfile), doc-build (ninja doc/src/sgml/postgres.html), and coverage-delta (gcov). S3 was refuted (accepted by design, not a defect requiring repair). The combined F-001/F-002/S2 repair patch (SHA256 d0a57deebbe1ed6f7d77977a2a1438b7bf3a3cba3a401c0250e757cd5ecdfacb) compiles clean, passes all 13 existing TAP subtests, and the doc-build contains the new "residual plaintext"/"out-of-memory" text. The F-003 test repair (SHA256 499c841be9ef2699ce1bad206ef0287ca6b7ed66b11c13a9727f0216c218dc2c) grows the suite to 15 subtests, all passing. The COV-1 test repair (SHA256 327daba51caf5c671e9e8c42e5ede6632db6a18ffb00625b974d2dca4f511516) also yields 15/15 subtests and covers the home-directory fallback path.

Repair Validation Summary

All patches apply cleanly on top of the staged diff (HEAD 0c5d6269614e107d1d2d669f82f63f7e232b30c9). All validation performed in disposable worktrees; the reviewed worktree was never modified.

Changed-Code Coverage

• Counter epoch: fresh (first and only test run on this coverage build)
• Execution lane: host-disposable (meson + ninja with -Db_coverage=true)
• Test commands: meson test libpq/007_passfile → 13/13 subtests passed
• Integrity errors: none
• Build root: disposable cov_build in a disposable worktree (outside reviewed worktree)

Raw metrics

Line coverage: 23/30 = 76.7%. Branch coverage: 13/20 = 65.0%.

Touched functions

Actionable uncovered regions

UR-1: fe-connect.c:8190,8192-8194 — home-directory fallback (requires-special-harness)

8190: 			char		homedir[MAXPGPATH];
8192: 			if (!pqGetHomeDirectory(homedir, sizeof(homedir)))
8193: 				return NULL;
8194: 			snprintf(pgpassfile, sizeof(pgpassfile), "%s/%s",

Why missed: the TAP test always supplies an explicit passfile or sets PGPASSFILE. The home-dir fallback (passfile=NULL/empty + PGPASSFILE unset) is never entered. Requires environment injection (controllable $HOME) and platform guards.

Test repair: platform-guarded TAP block (SKIP on Windows) with local %ENV deleting PGPASSFILE and setting HOME to a tempdir containing .pgpass, plus a no-HOME variant. Validated: 15/15 subtests; lines 8190, 8192, 8194 now covered; line 8193 remains uncovered (pqGetHomeDirectory-fails branch, infeasible without a UID with no passwd entry).

COV-1 finding issued (MINOR tests, verified-by-coverage) — home-directory fallback of PQpassfileLookup is untested. Repair patch SHA256: 327daba51caf5c671e9e8c42e5ede6632db6a18ffb00625b974d2dca4f511516.

UR-2: libpq_testclient.c:38-39 — usage error (coverable-now)

38: 		fprintf(stderr, "usage: libpq_testclient --passfile PASSFILE HOSTNAME PORT DBNAME USERNAME\n");
39: 		return 1;

Trivially coverable by calling libpq_testclient --passfile with too few args. Test example provided (TAP). Not issued as a standalone COV finding — test-harness usage guard, not production logic.

UR-3: libpq_testclient.c:69 — fallback message (coverable-now)

69: 	printf("currently only --ssl and --passfile are supported\n");

Coverable by calling with an unknown subcommand. Test example provided (TAP). Not a standalone COV finding — test-harness fallback, not production logic.

Uncovered branches

Exclusion totals

• excluded_changed_coordinates: none (all uncovered lines/branches are real semantic code with valid gcov coordinates)
• changed_lines_without_coverage_coordinates: 0

Non-instrumented changes

• doc/src/sgml/libpq.sgml — SGML documentation; no executable lines.
• src/interfaces/libpq/exports.txt — symbol export list; no executable lines.
• src/interfaces/libpq/libpq-fe.h — header; declarations only.
• src/interfaces/libpq/meson.build — build system; no executable lines.
• src/interfaces/libpq/t/007_passfile.pl — TAP test script; Perl, not C instrumentable.

Residual limitations

• Coverage reflects only libpq/007_passfile; no other test invokes PQpassfileLookup.
• Pre-existing --ssl branch of main (line 61) is out of patch scope and not counted.
• Branch percentages are gcov taken/fallthrough pct; covered/uncovered uses binary hit/miss (0% == uncovered).
• Line 8193 (return NULL when pqGetHomeDirectory fails) remains uncovered after COV-1 repair — infeasible without a UID with no passwd database entry.

pgsql-hackers thread ledger

• Status: no-thread-found
• Patch thread (657548): contains only the submission (b650e555-..., Diego Amell, 2026-09-04). No review replies.
• Precursor threads (649495, 654995): the portaddr proposal was rejected; Denis Smirnov suggested exposing passwordFromFile() as PQpassfileLookup(). Design origin corroborated, but no review of THIS patch.
• All findings: new relative to the thread (no on-list discussion of any finding), except S3 which is refuted (accepted by design).
• Ledger report slug: commitfest-7261-review-thread-ledger-r2

Aggregated Test Plan

Per-finding detail

S2 — MINOR security — credential residue

Returned password is malloc'd by strdup; PQfreemem is bare free() with no zeroing. Internal path uses explicit_bzero before free. Repair: add SGML note recommending explicit_bzero before PQfreemem. Part of combined patch.

S3 — NIT security — refuted (accepted by design)

Silent credential enumeration without a connection is the intended feature, guarded by the same file-permission checks (0600, plain-file). No fix required.

Deterministic verdict

• Verdict: ready-for-committer
• Rationale: No BLOCKERs, no MAJORs. All findings are MINOR or NIT. No live thread objections. No test-only MAJORs. The patch is correct, well-tested, well-documented, and architecturally sound. It faithfully reuses the existing audited passwordFromFile path with no security, memory, or API-contract regressions.
• Verified blocker split: 0 verified, 0 unverified.
• Follow-up: F-001 doc clarification, F-003 stronger port test, COV-1 home-dir fallback test, and S2 zeroing doc note can be applied as follow-up patches. S3 is accepted by design.

Provenance / model routes

Verified-clean section

No findings were refuted as false positives except S3, which is accepted by design (not a defect). All other findings were verified as real and carry concrete repair patches validated in disposable worktrees.