RE: [PATCH] libpq: Add PQpassfileLookup()

Hi Diego,

I reviewed v1 (CF 7261). Thanks for writing this, and thanks to Denis
for the original suggestion.

I think exposing the existing passfile lookup is a useful API for the
SSH-tunnel/dynamic-port case.

Just found a couple of issues and some smaller test/documentation points.

1. Error handling is lost by PQpassfileLookup()

The new function calls:

const char *errmsg;

return passwordFromFile(hostname, port, dbname, username,
passfile, &errmsg);

but never examines errmsg. passwordFromFile() uses it to distinguish
an allocation failure from an ordinary lookup miss. The connection
path turns that error into CONNECTION_BAD, whereas PQpassfileLookup()
returns NULL in both cases.

The documentation currently says:

NULL if no password could be found

which does not make the distinction clear.

I think the simplest option is to keep the proposed public signature
and document that NULL is returned if no matching password is found or
if the lookup cannot be completed, for example because of an allocation
failure. Alternatively, the public API would need an error output
parameter.

The local errmsg variable is still required by the current internal
passwordFromFile() signature unless that function is changed to permit
a NULL error pointer.

2. PGPASSFILE can be silently truncated

When passfile is NULL or empty, the wrapper copies PGPASSFILE into a
MAXPGPATH-sized buffer:

strlcpy(pgpassfile, pgpassenv, sizeof(pgpassfile));

A value longer than MAXPGPATH is therefore silently truncated. This
differs from normal connection option processing, where the value of
PGPASSFILE is dynamically allocated.

There is no need to copy the environment value here. passfile can
point directly to pgpassenv; the local buffer is needed only when
constructing the default path.

3. Test coverage for argument defaults

The TAP test covers matching, wildcards, first-match-wins, escaping,
PGPASSFILE, and the permissions check. A few behaviours documented by
the new API are not exercised:

* NULL and empty port should use DEF_PGPORT_STR. The current
localhost test uses a wildcard port, so it does not test that
default. The test should account for a non-standard compiled-in
port rather than assuming 5432.

* NULL dbname and username are tested, but their empty-string forms
are not.

* NULL passfile with PGPASSFILE is tested, but empty passfile and the
fallback to the default password file location are not. The test
client already preserves an empty argument, so the former only
needs another TAP case.

* The documented default-socket-directory-to-localhost rule is not
currently tested.

The last two may require some platform-specific handling, but the
default-port and empty-argument cases should be straightforward.

I am less sure about this test:

*:*:*:fallback:fallpw:anything after an unescaped colon is ignored

The documented format requires a colon in a field to be escaped.
Testing that trailing fields are silently ignored appears to preserve
an undocumented parser behaviour. Since that behaviour is inherited
from passwordFromFile() rather than introduced by this API, I would
prefer to leave it out of this patch.

A couple of smaller points:

* test_passfile_lookup() checks argc < 7 and silently accepts extra
arguments. argc != 7 would match the usage string more closely.

* The C comment describing hostname defaults does not mention the
default socket directory rule that is described in the SGML docs.

* The returned allocation contains a plaintext password.
PQfreemem() does not erase it, whereas libpq explicitly clears
passwords stored in PGconn before freeing them. It would be useful
for the documentation to mention that callers are responsible for
securely clearing the result where appropriate.

The rest looks good to me: reusing passwordFromFile(), the feature
macro, exports.txt entry, Meson integration, and the general test
structure all look appropriate. Connection establishment remains
unchanged, as intended.

I will mark the CF entry as Waiting on Author. Happy to review the
next version!

Best regards,
Yuriy Grigoryev