[PATCH v2] amcheck: Allow interrupting the child-level rightlink walk

[PATCH v2] amcheck: Allow interrupting the child-level rightlink walk

bt_child_highkey_check() walks right along the child level following
btpo_next links, reading a page on each iteration, but its loop lacked a
CHECK_FOR_INTERRUPTS().  Every other page-traversal loop in
verify_nbtree.c already has one.

The loop's existing checks do not guarantee a timely exit on a corrupt
index.  The in-loop cycle check only fires when a rightlink points back
through the block the walk started from, or to a page whose btpo_prev
points to itself, so a cycle further downstream can go undetected -- for
example a run of pages all flagged P_INCOMPLETE_SPLIT, which skips both
the high-key comparison and bt_downlink_missing_check() and so never
reaches an error.  Such an index, or simply a very long rightlink chain,
makes the walk effectively uninterruptible.  Since amcheck exists to be
run against possibly-corrupt indexes, make this walk respond to query
cancellation and shutdown requests like the sibling loops do.
---
v2: only the commit message changed -- v1's description of the in-loop
cycle check was imprecise.  The code (the one-line CHECK_FOR_INTERRUPTS)
is unchanged.

This is the same shape as commit 191dce109be ("contrib/amcheck: Add
heapam CHECK_FOR_INTERRUPTS()"), which was backpatched.
bt_child_highkey_check() exists in all supported branches, so this looks
like a backpatch candidate as well.

 contrib/amcheck/verify_nbtree.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/contrib/amcheck/verify_nbtree.c b/contrib/amcheck/verify_nbtree.c
index 3ef2d66f826..486bd7a87cc 100644
--- a/contrib/amcheck/verify_nbtree.c
+++ b/contrib/amcheck/verify_nbtree.c
@@ -2191,6 +2191,8 @@ bt_child_highkey_check(BtreeCheckState *state,
 	/* Move to the right on the child level */
 	while (true)
 	{
+		CHECK_FOR_INTERRUPTS();
+
 		/*
 		 * Did we traverse the whole tree level and this is check for pages to
 		 * the right of rightmost downlink?
-- 
2.50.1 (Apple Git-155)

Re: [PATCH] libpq: Add PQpassfileLookup()

Hi Denis,

Thank you, and thanks for the review that found the residue in the first
place.

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

Agreed on all four, and I had noticed two of them while checking 0001: on
an allocation failure, enlargePQExpBuffer() ends up in
markPQExpBufferBroken(), which free()s the block still holding the partially
read line and sets maxlen to 0, so the explicit_bzero(buf.data, buf.maxlen)
further down becomes a no-op on the replacement buffer.  I left it alone
because fixing it properly means touching pqexpbuffer.c, which is a
different patch and a different discussion.

The stdio buffer is the largest of the four by far, it can hold the whole
file, not just one line, and it is also the only one that is there on
every successful lookup rather than only on an error path.  If nobody
objects I will look at that one separately once this is settled, probably
with setvbuf() on an owned buffer that can be cleared before fclose().

I'll leave those out of this thread so they do not hold up the API.  Since
you and Yuriy are both happy with the series, I'll move the CF entry to
Ready for Committer.

Thanks,
Diego

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

Re: [PATCH v1] amcheck: Allow interrupting the child-level rightlink walk

Re: [PATCH] libpq: Add PQpassfileLookup()

Re: [PATCH] libpq: Add PQpassfileLookup()

Hi Diego,

Thanks for v2!

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


Best regards,
Denis Smirnov