Re: [PATCH] libpq: Add PQpassfileLookup()

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