Re: [PATCH] libpq: Add PQpassfileLookup()

Hi Diego,

Thanks for v3. Both patches look good to me. I have no further
comments.

One separate note for future work: even after 0001, copies of the
password can still remain in memory:

- The FILE buffer holds the whole .pgpass file, and fclose() frees
it without clearing it.
- On long lines, realloc() in enlargePQExpBuffer() can leave old
copies behind.
- markPQExpBufferBroken() also frees memory without clearing it
when an allocation fails.
- SCRAM and cleartext authentication make more copies later.

These issues already existed, and I don't see a practical way to
exploit them. They should not delay the new API. Maybe you would
like to look at them separately later.

Best regards,
Denis Smirnov

> On 16 Sep 2026, at 20:39, Diego <mrstephenamell(at)gmail(dot)com> wrote:
>
> Hi Denis, hi Yuriy,
>
> Thanks to both of you.v3 attached, now as a series of two patches:
>
> - v3-0001 fixes the residue Denis found, in passwordFromFile() itself.
> - v3-0002 is the API patch, rebased on 0001, with Yuriy's wording.
>
> > The new docs now say this, but passwordFromFile() leaves part of the
> > original password after removing escapes in place:
> > Password in .pgpass: pa\\ss\:word
> > Returned buffer:pa\ss:word\0d\0
> > A caller using explicit_bzero(password, strlen(password)) before
> > PQfreemem() leaves the final 'd' untouched. Could we zero this tail in
> > passwordFromFile() before returning? The caller does not know the
> > original allocation size.
>
> Confirmed, and it is a bit wider than the last character: strdup(t)
> copies the rest of the line, and the in-place loop only overwrites the
> de-escaped length, so everything after that point -- the tail of the
> escaped password and any further fields on the line -- survives past
> the terminator.With a line such as
>
> host:5432:db:user:pw:extra:fields:here
>
> the allocation ends up as "pw\0extra:fields:here\0".
>
> It is also not specific to the new function.Connection establishment
> stores the same allocation in conn->connhost[i].password, and
> pqReleaseConnHosts() clears it with explicit_bzero(p, strlen(p)), so the
> same bytes have been left behind in libpq's own cleanup: they have been
> there since the de-escaping was added in 8d15e3ec4fc (2011), and the
> explicit_bzero() that fails to reach them dates from 74a308cf522 (2019).
> That is why 0001 is a separate patch: it stands on its own against
> master, touches only passwordFromFile(), and could be back-patched if a
> committer thinks that is worth it -- I have no strong opinion either
> way.For what it is worth, it cherry-picks cleanly onto REL_19_STABLE;
> on REL_18_STABLE down to REL_14_STABLE the only conflict is the
> function's header comment, which is a single line there, and the code
> hunks apply.
>
> Rather than zeroing the tail after the fact, 0001 moves the existing
> de-escape loop above the strdup(), so it runs in place on the line
> buffer -- which is already cleared with explicit_bzero(buf.data,
> buf.maxlen) on every exit -- and strdup() then copies only the
> de-escaped password.The loop body is unchanged, the returned string
> is byte-identical, and the allocation is exactly strlen() + 1 by
> construction, so there is nothing a caller needs to know.If you
> would rather have a one-line explicit_bzero() of the tail instead,
> that is easy to do, but it keeps the oversized copy around and the
> documentation could not promise anything about strlen().
>
> 0002 depends on 0001: it does not apply on bare master, and the
> sentence in its docs and commit message about overwriting strlen()
> bytes is only true on top of it.Squashing the two is fine by me if a
> committer prefers that; if 0001 is dropped, those sentences go with it.
>
> How I checked it, on master @ bd124434333:
>
> * the libpq TAP suite and authentication/001_password, on 0001 alone
> and on the full series: all green (007_passfile now has 20
> subtests), no new compiler warnings, pgindent clean.
> * an out-of-tree corpus of 21 lookups against a password file that
> covers the escaping corners (your example, a lone trailing
> backslash, an escaped colon as the last character, an empty
> password, fields after the password, a 10 kB password, 3000 escaped
> colons, a CRLF line, a four-field line, a wildcard line) run
> through v2 and v3: identical output and exit code for all 21.
> * a small harness that searches for the expected leftover bytes past
> the terminator, within malloc_usable_size() and under
> MALLOC_PERTURB_ so untouched slack cannot be mistaken for data: on
> v2 it finds them in 5 of the 21 lines ("d" in your example,
> "extra:fields:here", 2999 bytes of the escaped-colon case); on v3
> it finds them in none.
>
> There is no in-tree test for this, because nothing public can observe
> bytes past the terminator without undefined behaviour; the corpus and
> the harness are outside the tree.
>
> With that, the sentence in the docs about clearing the result became a
> real contract: the string holds nothing but the password and libpq
> writes nothing past its terminating zero byte, so overwriting strlen()
> bytes before freeing it is sufficient.The comment above
> PQpassfileLookup() says the same.
>
> > Noticed one small wording issue in both the commit message and the
> > documentation.They say that PGPASSFILE is the only environment
> > variable consulted by PQpassfileLookup().Strictly speaking, when the
> > default password file location is used, pqGetHomeDirectory() consults
> > HOME on Unix.The new TAP test relies on this behavior as well.
> > Perhaps this could instead say:
> > Other libpq connection-parameter environment variables are not
> > applied to the lookup keys; in particular, PGHOST and PGPORT are
> > ignored.
>
> Right -- fixed with your sentence, verbatim, in both the docs and the
> commit message, and "the default password file location" in the docs
> now points at the pgpass section, which already covers HOME (and
> %APPDATA% on Windows).I also added a TAP case for an escaped colon
> at the end of the password.I did not add one for fields after the
> password, per your earlier point about not testing undocumented parser
> behaviour; the out-of-tree corpus above includes that line.
>
> One thing I expect to be asked, so let me say it up front: the new
> function has no error channel.A lookup that finds nothing, a missing
> or badly-permissioned file, no home directory, and an allocation
> failure all come back as NULL.That mirrors what connection
> establishment does when the password file yields nothing -- the connect
> path only turns the out-of-memory case into a hard error -- and it
> keeps the function a plain wrapper around the existing lookup.If an
> error out-parameter is preferred I can add one; I did not want to
> design more API than the use case needs.Relatedly, the default-file
> fallback in PQpassfileLookup() repeats a few lines of the connect path;
> I can factor a small static helper if that is wanted.
>
> Both patches apply on master in order, most recently checked against
> a4f18fd8f28.I'll leave the CF entry at Needs review.
>
> Thanks,
> Diego<v3-0002-libpq-Add-PQpassfileLookup.patch><v3-0001-libpq-Do-not-leave-password-residue-in-passwordFr.patch>