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 <[email protected]> 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>
