From b5ebef7c4154d2423c68195a30cbcb37a2393054 Mon Sep 17 00:00:00 2001 From: Tom Lane Date: Thu, 2 Jun 2005 21:03:25 +0000 Subject: [PATCH] Push enable/disable of notify and catchup interrupts all the way down to just around the bare recv() call that gets a command from the client. The former placement in PostgresMain was unsafe because the intermediate processing layers (especially SSL) use facilities such as malloc that are not necessarily re-entrant. Per report from counterstorm.com. --- src/backend/libpq/be-secure.c | 80 ++++++++++++++++++++++++++++++++-- src/backend/tcop/postgres.c | 82 ++++++++++++++++++++++++++--------- src/include/tcop/tcopprot.h | 4 +- 3 files changed, 142 insertions(+), 24 deletions(-) diff --git a/src/backend/libpq/be-secure.c b/src/backend/libpq/be-secure.c index f5657efae5..3d5ae65a40 100644 --- a/src/backend/libpq/be-secure.c +++ b/src/backend/libpq/be-secure.c @@ -11,7 +11,7 @@ * * * IDENTIFICATION - * $PostgreSQL: pgsql/src/backend/libpq/be-secure.c,v 1.56 2005/01/08 22:51:12 tgl Exp $ + * $PostgreSQL: pgsql/src/backend/libpq/be-secure.c,v 1.57 2005/06/02 21:03:17 tgl Exp $ * * Since the server static private key ($DataDir/server.key) * will normally be stored unencrypted so that the database @@ -79,7 +79,6 @@ #include #include #include -#include #include #include #include @@ -97,6 +96,8 @@ #include "libpq/libpq.h" #include "miscadmin.h" +#include "tcop/tcopprot.h" + #ifdef USE_SSL static DH *load_dh_file(int keylength); @@ -308,8 +309,14 @@ rloop: } else #endif + { + prepare_for_client_read(); + n = recv(port->sock, ptr, len, 0); + client_read_ended(); + } + return n; } @@ -410,6 +417,73 @@ wloop: /* SSL specific code */ /* ------------------------------------------------------------ */ #ifdef USE_SSL + +/* + * Private substitute BIO: this wraps the SSL library's standard socket BIO + * so that we can enable and disable interrupts just while calling recv(). + * We cannot have interrupts occurring while the bulk of openssl runs, + * because it uses malloc() and possibly other non-reentrant libc facilities. + * + * As of openssl 0.9.7, we can use the reasonably clean method of interposing + * a wrapper around the standard socket BIO's sock_read() method. This relies + * on the fact that sock_read() doesn't call anything non-reentrant, in fact + * not much of anything at all except recv(). If this ever changes we'd + * probably need to duplicate the code of sock_read() in order to push the + * interrupt enable/disable down yet another level. + */ + +static bool my_bio_initialized = false; +static BIO_METHOD my_bio_methods; +static int (*std_sock_read) (BIO *h, char *buf, int size); + +static int +my_sock_read(BIO *h, char *buf, int size) +{ + int res; + + prepare_for_client_read(); + + res = std_sock_read(h, buf, size); + + client_read_ended(); + + return res; +} + +static BIO_METHOD * +my_BIO_s_socket(void) +{ + if (!my_bio_initialized) + { + memcpy(&my_bio_methods, BIO_s_socket(), sizeof(BIO_METHOD)); + std_sock_read = my_bio_methods.bread; + my_bio_methods.bread = my_sock_read; + my_bio_initialized = true; + } + return &my_bio_methods; +} + +/* This should exactly match openssl's SSL_set_fd except for using my BIO */ +static int +my_SSL_set_fd(SSL *s, int fd) +{ + int ret=0; + BIO *bio=NULL; + + bio=BIO_new(my_BIO_s_socket()); + + if (bio == NULL) + { + SSLerr(SSL_F_SSL_SET_FD,ERR_R_BUF_LIB); + goto err; + } + BIO_set_fd(bio,fd,BIO_NOCLOSE); + SSL_set_bio(s,bio,bio); + ret=1; +err: + return(ret); +} + /* * Load precomputed DH parameters. * @@ -761,7 +835,7 @@ open_server_SSL(Port *port) close_SSL(port); return -1; } - if (!SSL_set_fd(port->ssl, port->sock)) + if (!my_SSL_set_fd(port->ssl, port->sock)) { ereport(COMMERROR, (errcode(ERRCODE_PROTOCOL_VIOLATION), diff --git a/src/backend/tcop/postgres.c b/src/backend/tcop/postgres.c index fcb93d847d..297eb75d89 100644 --- a/src/backend/tcop/postgres.c +++ b/src/backend/tcop/postgres.c @@ -8,7 +8,7 @@ * * * IDENTIFICATION - * $PostgreSQL: pgsql/src/backend/tcop/postgres.c,v 1.445 2005/06/01 23:27:03 momjian Exp $ + * $PostgreSQL: pgsql/src/backend/tcop/postgres.c,v 1.446 2005/06/02 21:03:24 tgl Exp $ * * NOTES * this is the "main" module of the postgres backend and @@ -111,6 +111,13 @@ static volatile sig_atomic_t got_SIGHUP = false; */ static bool xact_started = false; +/* + * Flag to indicate that we are doing the outer loop's read-from-client, + * as opposed to any random read from client that might happen within + * commands like COPY FROM STDIN. + */ +static bool DoingCommandRead = false; + /* * Flags to implement skip-till-Sync-after-error behavior for messages of * the extended query protocol. @@ -406,6 +413,52 @@ ReadCommand(StringInfo inBuf) return result; } +/* + * prepare_for_client_read -- set up to possibly block on client input + * + * This must be called immediately before any low-level read from the + * client connection. It is necessary to do it at a sufficiently low level + * that there won't be any other operations except the read kernel call + * itself between this call and the subsequent client_read_ended() call. + * In particular there mustn't be use of malloc() or other potentially + * non-reentrant libc functions. This restriction makes it safe for us + * to allow interrupt service routines to execute nontrivial code while + * we are waiting for input. + */ +void +prepare_for_client_read(void) +{ + if (DoingCommandRead) + { + /* Enable immediate processing of asynchronous signals */ + EnableNotifyInterrupt(); + EnableCatchupInterrupt(); + + /* Allow "die" interrupt to be processed while waiting */ + ImmediateInterruptOK = true; + + /* And don't forget to detect one that already arrived */ + QueryCancelPending = false; + CHECK_FOR_INTERRUPTS(); + } +} + +/* + * client_read_ended -- get out of the client-input state + */ +void +client_read_ended(void) +{ + if (DoingCommandRead) + { + ImmediateInterruptOK = false; + QueryCancelPending = false; /* forget any CANCEL signal */ + + DisableNotifyInterrupt(); + DisableCatchupInterrupt(); + } +} + /* * Parse a query string and pass it through the rewriter. @@ -2959,6 +3012,7 @@ PostgresMain(int argc, char *argv[], const char *username) * not in other exception-catching places since these interrupts * are only enabled while we wait for client input. */ + DoingCommandRead = false; DisableNotifyInterrupt(); DisableCatchupInterrupt(); @@ -3063,21 +3117,13 @@ PostgresMain(int argc, char *argv[], const char *username) } /* - * (2) deal with pending asynchronous NOTIFY from other backends, - * and enable async.c's signal handler to execute NOTIFY directly. - * Then set up other stuff needed before blocking for input. + * (2) Allow asynchronous signals to be executed immediately + * if they come in while we are waiting for client input. + * (This must be conditional since we don't want, say, reads on + * behalf of COPY FROM STDIN doing the same thing.) */ - QueryCancelPending = false; /* forget any earlier CANCEL - * signal */ - - EnableNotifyInterrupt(); - EnableCatchupInterrupt(); - - /* Allow "die" interrupt to be processed while waiting */ - ImmediateInterruptOK = true; - /* and don't forget to detect one that already arrived */ - QueryCancelPending = false; - CHECK_FOR_INTERRUPTS(); + QueryCancelPending = false; /* forget any earlier CANCEL signal */ + DoingCommandRead = true; /* * (3) read a command (loop blocks here) @@ -3087,11 +3133,7 @@ PostgresMain(int argc, char *argv[], const char *username) /* * (4) disable async signal conditions again. */ - ImmediateInterruptOK = false; - QueryCancelPending = false; /* forget any CANCEL signal */ - - DisableNotifyInterrupt(); - DisableCatchupInterrupt(); + DoingCommandRead = false; /* * (5) check for any other interesting events that happened while diff --git a/src/include/tcop/tcopprot.h b/src/include/tcop/tcopprot.h index 23764dd1f9..23b338e7ae 100644 --- a/src/include/tcop/tcopprot.h +++ b/src/include/tcop/tcopprot.h @@ -7,7 +7,7 @@ * Portions Copyright (c) 1996-2005, PostgreSQL Global Development Group * Portions Copyright (c) 1994, Regents of the University of California * - * $PostgreSQL: pgsql/src/include/tcop/tcopprot.h,v 1.73 2004/12/31 22:03:44 pgsql Exp $ + * $PostgreSQL: pgsql/src/include/tcop/tcopprot.h,v 1.74 2005/06/02 21:03:25 tgl Exp $ * * OLD COMMENTS * This file was created so that other c files could get the two @@ -59,6 +59,8 @@ extern bool assign_max_stack_depth(int newval, bool doit, GucSource source); extern void die(SIGNAL_ARGS); extern void quickdie(SIGNAL_ARGS); extern void authdie(SIGNAL_ARGS); +extern void prepare_for_client_read(void); +extern void client_read_ended(void); extern int PostgresMain(int argc, char *argv[], const char *username); extern void ResetUsage(void); extern void ShowUsage(const char *title);