Don't allow immediate interrupts during authentication anymore.

We used to handle authentication_timeout by setting
ImmediateInterruptOK to true during large parts of the authentication
phase of a new connection.  While that happens to work acceptably in
practice, it's not particularly nice and has ugly corner cases.

Previous commits converted the FE/BE communication to use latches and
implemented support for interrupt handling during both
send/recv. Building on top of that work we can get rid of
ImmediateInterruptOK during authentication, by immediately treating
timeouts during authentication as a reason to die. As die interrupts
are handled immediately during client communication that provides a
sensibly quick reaction time to authentication timeout.

Additionally add a few CHECK_FOR_INTERRUPTS() to some more complex
authentication methods. More could be added, but this already should
provides a reasonable coverage.

While it this overall increases the maximum time till a timeout is
reacted to, it greatly reduces complexity and increases
reliability. That seems like a overall win. If the increase proves to
be noticeable we can deal with those cases by moving to nonblocking
network code and add interrupt checking there.

Reviewed-By: Heikki Linnakangas

3 files changed, 25 insertions, 20 deletions

diff --git a/src/backend/libpq/auth.c b/src/backend/libpq/auth.c
index 3f3cf4485ac..2d6b1cbb6c7 100644
--- a/src/backend/libpq/auth.c
+++ b/src/backend/libpq/auth.c
@@ -306,13 +306,6 @@ ClientAuthentication(Port *port)
 	 */
 	hba_getauthmethod(port);
 
-	/*
-	 * Enable immediate response to SIGTERM/SIGINT/timeout interrupts. (We
-	 * don't want this during hba_getauthmethod() because it might have to do
-	 * database access, eg for role membership checks.)
-	 */
-	ImmediateInterruptOK = true;
-	/* And don't forget to detect one that already arrived */
 	CHECK_FOR_INTERRUPTS();
 
 	/*
@@ -566,9 +559,6 @@ ClientAuthentication(Port *port)
 		sendAuthRequest(port, AUTH_REQ_OK);
 	else
 		auth_failed(port, status, logdetail);
-
-	/* Done with authentication, so we should turn off immediate interrupts */
-	ImmediateInterruptOK = false;
 }
 
 
@@ -580,6 +570,8 @@ sendAuthRequest(Port *port, AuthRequest areq)
 {
 	StringInfoData buf;
 
+	CHECK_FOR_INTERRUPTS();
+
 	pq_beginmessage(&buf, 'R');
 	pq_sendint(&buf, (int32) areq, sizeof(int32));
 
@@ -613,6 +605,8 @@ sendAuthRequest(Port *port, AuthRequest areq)
 	 */
 	if (areq != AUTH_REQ_OK)
 		pq_flush();
+
+	CHECK_FOR_INTERRUPTS();
 }
 
 /*
@@ -851,6 +845,9 @@ pg_GSS_recvauth(Port *port)
 	do
 	{
 		pq_startmsgread();
+
+		CHECK_FOR_INTERRUPTS();
+
 		mtype = pq_getbyte();
 		if (mtype != 'p')
 		{
@@ -900,6 +897,8 @@ pg_GSS_recvauth(Port *port)
 			 maj_stat, min_stat,
 			 (unsigned int) port->gss->outbuf.length, gflags);
 
+		CHECK_FOR_INTERRUPTS();
+
 		if (port->gss->outbuf.length != 0)
 		{
 			/*
@@ -1396,6 +1395,9 @@ interpret_ident_response(const char *ident_response,
  *	IP addresses and port numbers are in network byte order.
  *
  *	But iff we're unable to get the information from ident, return false.
+ *
+ *	XXX: Using WaitLatchOrSocket() and doing a CHECK_FOR_INTERRUPTS() if the
+ *	latch was set would improve the responsiveness to timeouts/cancellations.
  */
 static int
 ident_inet(hbaPort *port)
@@ -1510,6 +1512,8 @@ ident_inet(hbaPort *port)
 	/* loop in case send is interrupted */
 	do
 	{
+		CHECK_FOR_INTERRUPTS();
+
 		rc = send(sock_fd, ident_query, strlen(ident_query), 0);
 	} while (rc < 0 && errno == EINTR);
 
@@ -1525,6 +1529,8 @@ ident_inet(hbaPort *port)
 
 	do
 	{
+		CHECK_FOR_INTERRUPTS();
+
 		rc = recv(sock_fd, ident_response, sizeof(ident_response) - 1, 0);
 	} while (rc < 0 && errno == EINTR);
 
@@ -2413,6 +2419,10 @@ CheckRADIUSAuth(Port *port)
 	 * call to select() with a timeout, since somebody can be sending invalid
 	 * packets to our port thus causing us to retry in a loop and never time
 	 * out.
+	 *
+	 * XXX: Using WaitLatchOrSocket() and doing a CHECK_FOR_INTERRUPTS() if
+	 * the latch was set would improve the responsiveness to
+	 * timeouts/cancellations.
 	 */
 	gettimeofday(&endtime, NULL);
 	endtime.tv_sec += RADIUS_TIMEOUT;
diff --git a/src/backend/libpq/be-secure-openssl.c b/src/backend/libpq/be-secure-openssl.c
index 25ee070f5df..d5f97122ff4 100644
--- a/src/backend/libpq/be-secure-openssl.c
+++ b/src/backend/libpq/be-secure-openssl.c
@@ -377,6 +377,11 @@ aloop:
 				/* not allowed during connection establishment */
 				Assert(!port->noblock);
 
+				/*
+				 * No need to care about timeouts/interrupts here. At this
+				 * point authentication_timeout still employs
+				 * StartupPacketTimeoutHandler() which directly exits.
+				 */
 				if (err == SSL_ERROR_WANT_READ)
 					waitfor = WL_SOCKET_READABLE;
 				else
diff --git a/src/backend/libpq/crypt.c b/src/backend/libpq/crypt.c
index 599b63a48b5..97be9443c04 100644
--- a/src/backend/libpq/crypt.c
+++ b/src/backend/libpq/crypt.c
@@ -47,13 +47,6 @@ md5_crypt_verify(const Port *port, const char *role, char *client_pass,
 	Datum		datum;
 	bool		isnull;
 
-	/*
-	 * Disable immediate interrupts while doing database access.  (Note we
-	 * don't bother to turn this back on if we hit one of the failure
-	 * conditions, since we can expect we'll just exit right away anyway.)
-	 */
-	ImmediateInterruptOK = false;
-
 	/* Get role info from pg_authid */
 	roleTup = SearchSysCache1(AUTHNAME, PointerGetDatum(role));
 	if (!HeapTupleIsValid(roleTup))
@@ -80,9 +73,6 @@ md5_crypt_verify(const Port *port, const char *role, char *client_pass,
 	if (*shadow_pass == '\0')
 		return STATUS_ERROR;	/* empty password */
 
-	/* Re-enable immediate response to SIGTERM/SIGINT/timeout interrupts */
-	ImmediateInterruptOK = true;
-	/* And don't forget to detect one that already arrived */
 	CHECK_FOR_INTERRUPTS();
 
 	/*