schema: review-report-v2
source: postgresql-master
slug: commitfest-7254-review
target: CommitFest 7254 — Allow bare library names for non-superuser LOAD
head: 6168c65ddcac4494fb292418cc774897791e0667
base: 6168c65ddcac4494fb292418cc774897791e0667
diff_sha256: e2e3d1b254040736f2ba40199ee5c4a54d2ed9482788b926166ad9a31629cea8
verdict: ready-for-committer (test follow-up required)
blocker_counts:
  verified: 0
  unverified: 0
model: glm-5.2:cloud

CommitFest 7254 — Review Report

Allow bare library names for non-superuser LOAD

Patch summary

This patch moves the $libdir/plugins/ bare-library-name auto-expansion from load_libraries() (src/backend/utils/init/miscinit.c) into load_file() (src/backend/utils/fmgr/dfmgr.c), so that the SQL LOAD command for non-superusers (utility.c:854, restricted = !superuser()) gains the same bare-name convenience that local_preload_libraries already had. After the patch, a non-superuser can write LOAD 'mylib'; and have it interpreted as LOAD '$libdir/plugins/mylib';. The security model is preserved: the expansion only fires for names with no directory separator, and the expanded result is still validated by check_restricted_library_name().

Three files changed (+19/-15): doc/src/sgml/ref/load.sgml (documentation update), src/backend/utils/fmgr/dfmgr.c (expansion moved here), src/backend/utils/init/miscinit.c (expansion removed here).

Verdict

ready-for-committer (test follow-up required)

• 0 BLOCKER (verified: 0, unverified: 0)
• 1 MAJOR test-only (TEST-1, verified) — test coverage gap for the headline behavior
• 1 MAJOR security (SEC-1, refuted — pre-existing, not widened by this patch)
• 1 MINOR correctness (LOG-1, verified) — cosmetic DEBUG1 log-output change
• 1 NIT docs (DOC-1, verified) — wording divergence

Per the deterministic verdict: no surviving BLOCKER; no surviving correctness/security MAJOR (SEC-1 refuted); one test-only MAJOR (TEST-1) ⇒ ready-for-committer (test follow-up required).

Findings

Verification Summary

Verified/unverified blocker split: 0 verified BLOCKER, 0 unverified BLOCKER.

Repair Validation Summary

• TEST-1: The proposed test patch applies cleanly to HEAD+CF-patch, builds,

and pg_regress --temp-instance create_function_c passes (All 1 tests passed). The three pinned ERROR lines confirm the expansion behavior is exercised. Methods: apply-check, diff-check, targeted-test.

• SEC-1: Refuted; no repair owed. The pre-existing hardening opportunity

(reject all-dots tails in check_restricted_library_name) is noted as a separate follow-up.

• LOG-1: The optional logging-only patch applies on top of the CF patch,

compiles clean, and restores the pre-patch DEBUG1 log content. Methods: apply-check, diff-check, compile, targeted-test. Recommended: accept and document instead.

• DOC-1: The optional harmonization patch applies cleanly; `xmllint

--noout --nonet` confirms no new SGML errors (13 pre-existing unchanged). Full jade/openjade doc-build was not available. Methods: apply-check, diff-check, static-check.

Changed-Code Coverage

Coverage requirement: required (changed instrumentable implementation in src/backend/utils/fmgr/dfmgr.c).

Coverage status: measured (fresh disposable counter epoch at HEAD 6168c65ddca + CF patch applied, gcov instrumentation on the two changed backend files).

Execution lane

• Selected lane: host-disposable
• Attempt 1 (host-disposable, passed): Disposable git worktree at

scratch/cov-build, configured with --enable-debug and gcov instrumentation (--coverage -fprofile-arcs -ftest-coverage in CFLAGS for the two changed backend files), built and installed. Fresh counter epoch: all .gcda deleted before test run. pg_regress --temp-instance create_function_c (with verifier's proposed test patch applied): ok 1 - create_function_c 48 ms / All 1 tests passed. gcov -b -l produced valid .gcov files.

Test commands

make -C src/test/regress regress.so
cp src/test/regress/regress.so <pkglibdir>/regress.so
pg_regress --inputdir=. --temp-instance=<tmp> --bindir=<bindir> --dlpath=<pkglibdir> --temp-config=/dev/null create_function_c

Raw metrics

Integrity: integrity_errors: [] (empty list, no corrupted counters).

Uncovered regions

src/backend/utils/fmgr/dfmgr.c:176 — load_file — pfree(expanded)

• coverability: coverable-now
• reason: Line 176 (pfree(expanded)) is reached only when the expansion

branch fired (restricted=true AND first_dir_separator(filename)==NULL) AND load_file returned successfully (no ereport ERROR from check_restricted_library_name, expand_dynamic_library_name, or internal_load_library). The test exercises the expansion but uses non-existent library names, so internal_load_library ereports ERROR before reaching pfree(fullname)/pfree(expanded). A positive test that successfully loads a library from $libdir/plugins/ as a non-superuser would cover this line.

• test example: Copy regress.so (or a dedicated test plugin) to

$libdir/plugins/testplug.so, then as a non-superuser: SET session authorization regress_loaduser; LOAD 'testplug'; RESET session authorization; — this successfully loads $libdir/plugins/testplug.so, reaching pfree(expanded) at line 176.

Uncovered branches

src/backend/utils/fmgr/dfmgr.c:175 — load_file — if (expanded) true branch

• branch_id: 1, count: 0
• coverability: coverable-now
• reason: The true branch of if (expanded) at line 175 (fallthrough to

pfree(expanded) at 176) is never taken because the only test reaching line 175 uses non-existent libraries that ereport ERROR before returning. The true branch requires a successful restricted bare-name load.

• test example: Same positive restricted-load test as above (copy

regress.so to $libdir/plugins/, LOAD 'testplug' as non-superuser).

Touched functions

Non-instrumented changes

• doc/src/sgml/ref/load.sgml — Documentation SGML; not instrumentable C

implementation. No gcov coverage coordinates apply.

Residual limitations

The coverage measurement used the existing create_function_c regression test plus the verifier's proposed test patch (which exercises the restricted bare-name expansion with non-existent libraries). The one uncovered line (176) and one uncovered branch (line 175 true branch) require a positive restricted-load test — loading an actual library from $libdir/plugins/ as a non-superuser — which is not in the current test patch but is straightforward to add by copying regress.so into $libdir/plugins/. The miscinit.c change is a pure deletion (0 added executable lines), so no coverage gap exists there.

pgsql-hackers Thread Ledger

Status: reconciled Ledger report: commitfest-7254-review-thread-ledger

The thread ([PATCH] Allow bare library names for non-superuser LOAD, thread-id 657405) consists of 4 messages between the patch author (ChangAo Chen, cca5507@qq.com) and one reviewer (Japin Li, japinli@hotmail.com), all on 2026-09-03. Japin Li examined the patch and said "it looks good," noting the $libdir/plugins auto-expansion was added in commit abc3120e9b2 for local_preload_libraries but not LOAD, and it was unclear if that was intentional. No committer has replied. No objections. None of this review's findings (TEST-1, SEC-1, LOG-1, DOC-1) were previously raised on the thread; all are new.

Finding dispositions: all 4 findings → new (not previously raised).

Aggregated Test Plan

1. TEST-1 (blocking test follow-up): Apply the proposed test patch

adding non-superuser LOAD bare-name expansion coverage to create_function_c.sql/.out. Vehicle: pg_regress create_function_c. Validated: All 1 tests passed.

1. Coverage gap (line 176/branch 175): Optionally add a positive

restricted-load test (copy regress.so to $libdir/plugins/, LOAD as non-superuser) to cover the successful-load path through pfree(expanded).

1. LOG-1: No test owed (cosmetic DEBUG1-only change; waived).
2. DOC-1: No test owed (documentation prose; waived).
3. SEC-1: Refuted; no test owed.

Verified-clean section

• Memory safety / UAF / double-free: expanded is psprintf-allocated

in CurrentMemoryContext; expand_dynamic_library_name returns its own fresh allocation independent of expanded; pfree(fullname) does not double-free; pfree(expanded) is guarded by if (expanded). On ERROR paths the enclosing context is reset. Clean.

• No double-expansion across callers: load_libraries no longer expands;

load_file expands at most once. Replication callers pass restricted=false (unaffected).

• Path traversal: The expansion fires only when

first_dir_separator(filename)==NULL. The expanded string is validated by check_restricted_library_name. Names with any separator are not expanded and must already begin with $libdir/plugins/. Windows backslash tricks equally blocked (first_dir_separator treats both / and \). Clean.

• Privilege boundary: restricted is set solely by !superuser() at

utility.c:854 (SQL LOAD) and literal true for local_preload_libraries. No path sets restricted from user-controllable input. Clean.

• No SQL injection / no search_path / no SECURITY DEFINER surface.

Clean.

• dynamic_library_path GUC NOT consulted on the restricted path: After

expansion, the post-expansion string contains a separator, so expand_dynamic_library_name takes the have_slash=true branch and calls substitute_path_macro directly — never find_in_path. The user-controlled dynamic_library_path cannot redirect a restricted LOAD. Clean.

Provenance / model routes

• pg-patch-reviewer: glm-5.2:cloud — handoff report

commitfest-7254-review-handoff-patch-reviewer

• pg-security-reviewer: glm-5.2:cloud — handoff report

commitfest-7254-review-handoff-security-reviewer

• pg-finding-verifier: glm-5.2:cloud — handoff report

commitfest-7254-review-handoff-verifier

• Coverage: host-disposable lane, gcov instrumentation, fresh counter

epoch at HEAD 6168c65ddca + CF patch applied.

• Thread reconciliation: BM25 mail search + chronological assembly.
• Validator: run-scoped validate_review_artifact.py (SHA-256 bound)

— exit 0, "ok: pg-review-artifact-v2 is structurally complete".

Residual risk

Low. The code change is small, correct, memory-safe, and security-preserving. The only blocking follow-up is the test coverage gap (TEST-1), which has a validated proposed test patch ready to apply. The optional LOG-1 and DOC-1 repairs are cosmetic. The SEC-1 pre-existing hardening opportunity is noted for a separate commit.