Shouldn't GSSAPI and SSL code use FeBeWaitSet?
Hi,
While working on a patch to reuse a common WaitEventSet for latch
waits, I noticed that be-secure-gssapi.c and be-secure-openssl.c don't
use FeBeWaitSet. They probably should, for consistency with
be-secure.c, because that surely holds the socket they want, no? The
attached patch passes the "ssl" and "kerberos" tests and reaches that
code, confirmed by adding some log messages.
I'm not actually sure what the rationale is for reporting "terminating
connection due to unexpected postmaster exit" here but not elsewhere.
I copied the error from be-secure.c.
Attachments:
0001-Use-FeBeWaitSet-for-GSSAPI-and-SSL-waits.patchtext/x-patch; charset=US-ASCII; name=0001-Use-FeBeWaitSet-for-GSSAPI-and-SSL-waits.patchDownload
From d62e9337f201c7bed8040d72ff249900cf15f169 Mon Sep 17 00:00:00 2001
From: Thomas Munro <thomas.munro@gmail.com>
Date: Fri, 21 Feb 2020 21:47:13 +1300
Subject: [PATCH] Use FeBeWaitSet for GSSAPI and SSL waits.
These routines are waiting on the FE/BE socket, so they should
use the existing WaitEventSet for that.
---
src/backend/libpq/be-secure-gssapi.c | 37 +++++++++++++++++++++------
src/backend/libpq/be-secure-openssl.c | 19 +++++++++++---
2 files changed, 44 insertions(+), 12 deletions(-)
diff --git a/src/backend/libpq/be-secure-gssapi.c b/src/backend/libpq/be-secure-gssapi.c
index c25cfda0db..8ec5130cc5 100644
--- a/src/backend/libpq/be-secure-gssapi.c
+++ b/src/backend/libpq/be-secure-gssapi.c
@@ -379,7 +379,7 @@ be_gssapi_read(Port *port, void *ptr, size_t len)
/*
* Read the specified number of bytes off the wire, waiting using
- * WaitLatchOrSocket if we would block.
+ * FeBeWaitSet if we would block.
*
* Results are read into PqGSSRecvBuffer.
*
@@ -415,9 +415,19 @@ read_or_wait(Port *port, ssize_t len)
*/
if (ret <= 0)
{
- WaitLatchOrSocket(MyLatch,
- WL_SOCKET_READABLE | WL_EXIT_ON_PM_DEATH,
- port->sock, 0, WAIT_EVENT_GSS_OPEN_SERVER);
+ WaitEvent event;
+
+ /* FeBeWaitSet has port->sock in position 0 */
+ Assert(port == MyProcPort);
+ ModifyWaitEvent(FeBeWaitSet, 0, WL_SOCKET_READABLE, NULL);
+ WaitEventSetWait(FeBeWaitSet, -1, &event, 1,
+ WAIT_EVENT_GSS_OPEN_SERVER);
+
+ /* see comment in secure_read() */
+ if (event.events & WL_POSTMASTER_DEATH)
+ ereport(FATAL,
+ (errcode(ERRCODE_ADMIN_SHUTDOWN),
+ errmsg("terminating connection due to unexpected postmaster exit")));
/*
* If we got back zero bytes, and then waited on the socket to be
@@ -454,7 +464,7 @@ read_or_wait(Port *port, ssize_t len)
*
* Note that unlike the be_gssapi_read/be_gssapi_write functions, this
* function WILL block on the socket to be ready for read/write (using
- * WaitLatchOrSocket) as appropriate while establishing the GSSAPI
+ * FeBeWaitSet) as appropriate while establishing the GSSAPI
* session.
*/
ssize_t
@@ -595,9 +605,20 @@ secure_open_gssapi(Port *port)
/* Wait and retry if we couldn't write yet */
if (ret <= 0)
{
- WaitLatchOrSocket(MyLatch,
- WL_SOCKET_WRITEABLE | WL_EXIT_ON_PM_DEATH,
- port->sock, 0, WAIT_EVENT_GSS_OPEN_SERVER);
+ WaitEvent event;
+
+ /* FeBeWaitSet has port->sock in position 0 */
+ Assert(port == MyProcPort);
+ ModifyWaitEvent(FeBeWaitSet, 0, WL_SOCKET_WRITEABLE, NULL);
+ WaitEventSetWait(FeBeWaitSet, -1, &event, 1,
+ WAIT_EVENT_GSS_OPEN_SERVER);
+
+ /* see comment in secure_read() */
+ if (event.events & WL_POSTMASTER_DEATH)
+ ereport(FATAL,
+ (errcode(ERRCODE_ADMIN_SHUTDOWN),
+ errmsg("terminating connection due to unexpected postmaster exit")));
+
continue;
}
diff --git a/src/backend/libpq/be-secure-openssl.c b/src/backend/libpq/be-secure-openssl.c
index 18d3fff068..fe5c904fbf 100644
--- a/src/backend/libpq/be-secure-openssl.c
+++ b/src/backend/libpq/be-secure-openssl.c
@@ -347,6 +347,7 @@ be_tls_open_server(Port *port)
int err;
int waitfor;
unsigned long ecode;
+ WaitEvent event;
Assert(!port->ssl);
Assert(!port->peer);
@@ -415,12 +416,22 @@ aloop:
* StartupPacketTimeoutHandler() which directly exits.
*/
if (err == SSL_ERROR_WANT_READ)
- waitfor = WL_SOCKET_READABLE | WL_EXIT_ON_PM_DEATH;
+ waitfor = WL_SOCKET_READABLE;
else
- waitfor = WL_SOCKET_WRITEABLE | WL_EXIT_ON_PM_DEATH;
+ waitfor = WL_SOCKET_WRITEABLE;
+
+ /* FeBeWaitSet has port->sock in position 0 */
+ Assert(port == MyProcPort);
+ ModifyWaitEvent(FeBeWaitSet, 0, waitfor, NULL);
+ (void) WaitEventSetWait(FeBeWaitSet, -1, &event, 1,
+ WAIT_EVENT_SSL_OPEN_SERVER);
+
+ /* see comment in secure_read() */
+ if (event.events & WL_POSTMASTER_DEATH)
+ ereport(FATAL,
+ (errcode(ERRCODE_ADMIN_SHUTDOWN),
+ errmsg("terminating connection due to unexpected postmaster exit")));
- (void) WaitLatchOrSocket(MyLatch, waitfor, port->sock, 0,
- WAIT_EVENT_SSL_OPEN_SERVER);
goto aloop;
case SSL_ERROR_SYSCALL:
if (r < 0)
--
2.20.1
On Mon, Feb 24, 2020 at 4:49 PM Thomas Munro <thomas.munro@gmail.com> wrote:
While working on a patch to reuse a common WaitEventSet for latch
waits, I noticed that be-secure-gssapi.c and be-secure-openssl.c don't
use FeBeWaitSet. They probably should, for consistency with
be-secure.c, because that surely holds the socket they want, no?
Hmm. Perhaps it's like that because they're ignoring their latch
(though they pass it in just because that interface requires it). So
then why not reset it and process read interrupts, like be-secure.c?
On Mon, Feb 24, 2020 at 4:55 PM Thomas Munro <thomas.munro@gmail.com> wrote:
On Mon, Feb 24, 2020 at 4:49 PM Thomas Munro <thomas.munro@gmail.com> wrote:
While working on a patch to reuse a common WaitEventSet for latch
waits, I noticed that be-secure-gssapi.c and be-secure-openssl.c don't
use FeBeWaitSet. They probably should, for consistency with
be-secure.c, because that surely holds the socket they want, no?Hmm. Perhaps it's like that because they're ignoring their latch
(though they pass it in just because that interface requires it). So
then why not reset it and process read interrupts, like be-secure.c?
Perhaps the theory is that it doesn't matter if they ignore eg
SIGQUIT, because authentication_timeout will come along in (say) 60
seconds and exit anyway? That makes me wonder whether it's OK that
StartupPacketTimeoutHandler() does proc_exit() from a signal handler.