From 6647248e3708843be93c7ca670cd219fe8e61026 Mon Sep 17 00:00:00 2001 From: Andres Freund Date: Tue, 3 Feb 2015 22:54:48 +0100 Subject: [PATCH] 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 --- src/backend/libpq/auth.c | 30 ++++++++++++++++++--------- src/backend/libpq/be-secure-openssl.c | 5 +++++ src/backend/libpq/crypt.c | 10 --------- src/backend/tcop/postgres.c | 17 +++++---------- src/backend/utils/init/postinit.c | 16 +++++++++----- 5 files changed, 41 insertions(+), 37 deletions(-) diff --git a/src/backend/libpq/auth.c b/src/backend/libpq/auth.c index 3f3cf4485a..2d6b1cbb6c 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 25ee070f5d..d5f97122ff 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 599b63a48b..97be9443c0 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(); /* diff --git a/src/backend/tcop/postgres.c b/src/backend/tcop/postgres.c index 7e9408e61d..63573046a9 100644 --- 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 983b237d7a..66aa7ea61b 100644 --- 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)