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

diff --git a/src/backend/libpq/auth.c b/src/backend/libpq/auth.c
index 3f3cf4485ac5ec62c82b33e339264e8bfe680918..2d6b1cbb6c7c3ae3ece6812f9550fe38e0465af4 100644 (file)
--- 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 25ee070f5dfb28d4d6a5f31b9ff546f871389489..d5f97122ff408f847bb59cabf48f6e51db3e2edd 100644 (file)
--- 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 599b63a48b5ee75c29c3bab72dcca32fa8740644..97be9443c04d88fb1bf340f04ce1361322211001 100644 (file)
--- 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();
 
        /*

diff --git a/src/backend/tcop/postgres.c b/src/backend/tcop/postgres.c
index 7e9408e61d9312ea2abffa0407f45f618934bc0c..63573046a9e2b06aec284a6e55e6765716bdf460 100644 (file)
--- a/src/backend/tcop/postgres.c
+++ b/src/backend/tcop/postgres.c
@@ -2880,7 +2880,11 @@ ProcessInterrupts(void)
                /* As in quickdie, don't risk sending to client during auth */
                if (ClientAuthInProgress && whereToSendOutput == DestRemote)
                        whereToSendOutput = DestNone;
-               if (IsAutoVacuumWorkerProcess())
+               if (ClientAuthInProgress)
+                       ereport(FATAL,
+                                       (errcode(ERRCODE_QUERY_CANCELED),
+                                        errmsg("canceling authentication due to timeout")));
+               else if (IsAutoVacuumWorkerProcess())
                        ereport(FATAL,
                                        (errcode(ERRCODE_ADMIN_SHUTDOWN),
                                         errmsg("terminating autovacuum process due to administrator command")));
@@ -2959,17 +2963,6 @@ ProcessInterrupts(void)
                }
 
                QueryCancelPending = false;
-               if (ClientAuthInProgress)
-               {
-                       ImmediateInterruptOK = false;           /* not idle anymore */
-                       LockErrorCleanup();
-                       /* As in quickdie, don't risk sending to client during auth */
-                       if (whereToSendOutput == DestRemote)
-                               whereToSendOutput = DestNone;
-                       ereport(ERROR,
-                                       (errcode(ERRCODE_QUERY_CANCELED),
-                                        errmsg("canceling authentication due to timeout")));
-               }
 
                /*
                 * If LOCK_TIMEOUT and STATEMENT_TIMEOUT indicators are both set, we

diff --git a/src/backend/utils/init/postinit.c b/src/backend/utils/init/postinit.c
index 983b237d7a17b3a157299da414f930bb3482f38f..66aa7ea61b635706e1b9b73c1f9aa9a32c43f321 100644 (file)
--- a/src/backend/utils/init/postinit.c
+++ b/src/backend/utils/init/postinit.c
@@ -1099,18 +1099,24 @@ ShutdownPostgres(int code, Datum arg)
 static void
 StatementTimeoutHandler(void)
 {
+       int sig = SIGINT;
+
+       /*
+        * During authentication the timeout is used to deal with
+        * authentication_timeout - we want to quit in response to such timeouts.
+        */
+       if (ClientAuthInProgress)
+               sig = SIGTERM;
+
 #ifdef HAVE_SETSID
        /* try to signal whole process group */
-       kill(-MyProcPid, SIGINT);
+       kill(-MyProcPid, sig);
 #endif
-       kill(MyProcPid, SIGINT);
+       kill(MyProcPid, sig);
 }
 
 /*
  * LOCK_TIMEOUT handler: trigger a query-cancel interrupt.
- *
- * This is identical to StatementTimeoutHandler, but since it's so short,
- * we might as well keep the two functions separate for clarity.
  */
 static void
 LockTimeoutHandler(void)