Re: [PATCH] libpq: Add PQpassfileLookup()

Hi Jurij,

Thanks for the great review! v2 attached: all your points addressed,
plus a rebase onto current master.

> 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.

Done the simple way: the docs and the function's comment now say NULL
means no matching password was found or the lookup could not be
completed. I also took your other suggestion and made
passwordFromFile() accept a NULL error pointer, so the wrapper no
longer needs a dummy variable.

> 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.

Done exactly that way, the truncation is gone.

> * 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.

Added. The test client grew a --passfile-defaults mode that prints
DEF_PGPORT_STR and DEFAULT_PGSOCKET_DIR, and the TAP test writes its
passfile entry under whatever port that reports, so no 5432 assumption
anywhere. NULL and empty port are both covered. I tried it against a
build configured with -Dpgport=5499 and it passes there too.

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

Added both. Small deviation here: the test client maps an argument of
"=" to an empty string instead of taking a real empty argument. No
TAP test in the tree passes an empty argv element today, and I did not
want to find out on the Windows CI whether one survives process
spawning there.

> * NULL passfile with PGPASSFILE is tested, but empty passfile and the
> fallback to the default password file location are not.

Added both: an empty passfile falling back to PGPASSFILE, and a NULL
passfile with no PGPASSFILE falling back to ~/.pgpass, by pointing
HOME at a temporary directory (skipped on Windows, where the default
location does not come from the environment).

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

Added, using the socket directory reported by --passfile-defaults
(skipped if the platform reports none).

> 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.

Agreed, I removed the test case and its passfile line.

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

Changed.

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

Added.

> * 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.

Added a paragraph to the docs: the result is a cleartext password,
PQfreemem() does not erase it, and callers that do not want it
lingering in memory should overwrite it before freeing.

v2 also wraps the doc synopsis like the neighboring entries do, and
now spells out that PGPASSFILE is the only environment variable the
function looks at (PGHOST and PGPORT are not applied to the lookup
keys).

I'll move the CF entry back to Needs review.

Thank you,
BR,
Diego