public inbox for [email protected]
help / color / mirror / Atom feedFrom: Jacob Champion <[email protected]>
To: Daniel Gustafsson <[email protected]>
Cc: Heikki Linnakangas <[email protected]>
Cc: Andres Freund <[email protected]>
Cc: Postgres hackers <[email protected]>
Cc: Michael Paquier <[email protected]>
Cc: Andrew Dunstan <[email protected]>
Cc: Stephen Frost <[email protected]>
Cc: Thomas Munro <[email protected]>
Subject: Re: Support for NSS as a libpq TLS backend
Date: Thu, 12 Nov 2020 22:12:42 +0000
Message-ID: <[email protected]> (raw)
In-Reply-To: <[email protected]>
References: <[email protected]>
<[email protected]>
<CAA8=A7-suZg90M7Fgyo1Rfymkrpdh_Z547RKXrbHirsAmojQSw@mail.gmail.com>
<[email protected]>
<[email protected]>
<[email protected]>
<[email protected]>
<[email protected]>
<[email protected]>
<[email protected]>
<[email protected]>
<[email protected]>
<[email protected]>
<[email protected]>
<[email protected]>
<[email protected]>
<[email protected]>
<[email protected]>
<[email protected]>
<[email protected]>
On Nov 11, 2020, at 10:57 AM, Jacob Champion <[email protected]> wrote:
>
> False alarm -- the stderr debugging I'd added in to track down the
> assertion tripped up the "no stderr" tests. Zero failing tests now.
I took a look at the OpenSSL interop problems you mentioned upthread. I
don't see a hang like you did, but I do see a PR_IO_TIMEOUT_ERROR during
connection.
I think pgtls_read() needs to treat PR_IO_TIMEOUT_ERROR as if no bytes
were read, in order to satisfy its API. There was some discussion on
this upthread:
On Oct 27, 2020, at 1:07 PM, Daniel Gustafsson <[email protected]> wrote:
>
> On 20 Oct 2020, at 21:15, Andres Freund <[email protected]> wrote:
>>
>>> + case PR_IO_TIMEOUT_ERROR:
>>> + break;
>>
>> What does this mean? We'll return with a 0 errno here, right? When is
>> this case reachable?
>
> It should, AFAICT, only be reachable when PR_Recv is used with a timeout which
> we don't do. It mentioned somewhere that it had happened in no-wait calls due
> to a bug, but I fail to find that reference now. Either way, I've removed it
> to fall into the default error handling which now sets errno correctly as that
> was a paddle short here.
PR_IO_TIMEOUT_ERROR is definitely returned in no-wait calls on my
machine. It doesn't look like the PR_Recv() API has a choice -- if
there's no data, it can't return a positive integer, and returning zero
means that the socket has been disconnected. So -1 with a timeout error
is the only option.
I'm not completely sure why this is exposed so easily with an OpenSSL
server -- I'm guessing the implementation slices up its packets
differently on the wire, causing a read event before NSS is able to
decrypt a full record -- but it's worth noting that this case also shows
up during NSS-to-NSS psql connections, when handling notifications at
the end of every query. PQconsumeInput() reports a hard failure with the
current implementation, but its return value is ignored by
PrintNotifications(). Otherwise this probably would have showed up
earlier.
(What's the best way to test this case? Are there lower-level tests for
the protocol/network layer somewhere that I'm missing?)
While patching this case, I also noticed that pgtls_read() doesn't call
SOCK_ERRNO_SET() for the disconnection case. That is also in the
attached patch.
--Jacob
Attachments:
[application/octet-stream] nss-handle-timeouts-and-disconnections-in-pgtls_read.patch (2.0K, ../[email protected]/2-nss-handle-timeouts-and-disconnections-in-pgtls_read.patch)
download | inline diff:
From 4bc5dc61e8be490b013be5f9b3326faae2dc99c6 Mon Sep 17 00:00:00 2001
From: Jacob Champion <[email protected]>
Date: Thu, 12 Nov 2020 12:02:39 -0800
Subject: [PATCH] nss: handle timeouts and disconnections in pgtls_read
PR_IO_TIMEOUT_ERROR was being treated as a hard error, but since we're
explicitly asking PR_Recv() not to wait, we want to treat it as "zero
bytes read" instead.
This case happens relatively often in psql, during PQconsumeInput() --
but the return value of PQconsumeInput() is being ignored during
notification handling, which masks the problem. This also seems to fix
interoperability with OpenSSL servers on my machine, possibly due to
differences in message fragmentation between the implementations?
Also ensure that SOCK_ERRNO_SET() is called in the disconnection case;
read_errno was being set but not used.
---
src/interfaces/libpq/fe-secure-nss.c | 19 +++++++++++++------
1 file changed, 13 insertions(+), 6 deletions(-)
diff --git a/src/interfaces/libpq/fe-secure-nss.c b/src/interfaces/libpq/fe-secure-nss.c
index 3090f37f5e..8fc8e138e2 100644
--- a/src/interfaces/libpq/fe-secure-nss.c
+++ b/src/interfaces/libpq/fe-secure-nss.c
@@ -429,15 +429,19 @@ pgtls_read(PGconn *conn, void *ptr, size_t len)
if (nread == 0)
{
read_errno = ECONNRESET;
- return -1;
+ nread = -1;
}
-
- if (nread == -1)
+ else if (nread == -1)
{
status = PR_GetError();
switch (status)
{
+ case PR_IO_TIMEOUT_ERROR:
+ /* No data available yet. */
+ nread = 0;
+ break;
+
case PR_WOULD_BLOCK_ERROR:
read_errno = EWOULDBLOCK;
break;
@@ -456,9 +460,12 @@ pgtls_read(PGconn *conn, void *ptr, size_t len)
break;
}
- printfPQExpBuffer(&conn->errorMessage,
- libpq_gettext("TLS read error: %s"),
- pg_SSLerrmessage(status));
+ if (nread == -1)
+ {
+ printfPQExpBuffer(&conn->errorMessage,
+ libpq_gettext("TLS read error: %s"),
+ pg_SSLerrmessage(status));
+ }
}
SOCK_ERRNO_SET(read_errno);
--
2.24.1
view thread (181+ messages) latest in thread
reply
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Reply to all the recipients using the --to and --cc options:
reply via email
To: [email protected]
Cc: [email protected], [email protected], [email protected], [email protected], [email protected], [email protected], [email protected], [email protected], [email protected]
Subject: Re: Support for NSS as a libpq TLS backend
In-Reply-To: <[email protected]>
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
This inbox is served by agora; see mirroring instructions
for how to clone and mirror all data and code used for this inbox