From 5d7a555d0f42bce2a3ac05ea4fc32e6781803f1f Mon Sep 17 00:00:00 2001 From: Tom Lane Date: Thu, 2 Dec 2004 23:20:21 +0000 Subject: [PATCH] Code review for recent libpq changes. Be more careful about error handling in SIGPIPE processing; avoid unnecessary pollution of application link-symbol namespace; spell 'pointer to function' in the conventional way. --- src/interfaces/libpq/fe-connect.c | 31 +++++------ src/interfaces/libpq/fe-print.c | 9 ++-- src/interfaces/libpq/fe-secure.c | 87 ++++++++++++++++++++----------- src/interfaces/libpq/libpq-fe.h | 13 +++-- src/interfaces/libpq/libpq-int.h | 21 ++++---- 5 files changed, 90 insertions(+), 71 deletions(-) diff --git a/src/interfaces/libpq/fe-connect.c b/src/interfaces/libpq/fe-connect.c index 30e56b4b6e..57d7ee8e6f 100644 --- a/src/interfaces/libpq/fe-connect.c +++ b/src/interfaces/libpq/fe-connect.c @@ -8,7 +8,7 @@ * * * IDENTIFICATION - * $PostgreSQL: pgsql/src/interfaces/libpq/fe-connect.c,v 1.291 2004/12/02 15:32:54 momjian Exp $ + * $PostgreSQL: pgsql/src/interfaces/libpq/fe-connect.c,v 1.292 2004/12/02 23:20:19 tgl Exp $ * *------------------------------------------------------------------------- */ @@ -202,6 +202,11 @@ static int parseServiceInfo(PQconninfoOption *options, static char *pwdfMatchesString(char *buf, char *token); static char *PasswordFromFile(char *hostname, char *port, char *dbname, char *username); +static void default_threadlock(int acquire); + + +/* global variable because fe-auth.c needs to access it */ +pgthreadlock_t pg_g_threadlock = default_threadlock; /* @@ -3276,16 +3281,6 @@ PasswordFromFile(char *hostname, char *port, char *dbname, char *username) * if they are not required. */ -void -PQinitSSL(int do_init) -{ -#ifdef USE_SSL - pq_initssllib = do_init; -#endif -} - -static pgthreadlock_t default_threadlock; - static void default_threadlock(int acquire) { @@ -3313,17 +3308,15 @@ default_threadlock(int acquire) #endif } -pgthreadlock_t *g_threadlock = default_threadlock; - -pgthreadlock_t * -PQregisterThreadLock(pgthreadlock_t *newhandler) +pgthreadlock_t +PQregisterThreadLock(pgthreadlock_t newhandler) { - pgthreadlock_t *prev; + pgthreadlock_t prev = pg_g_threadlock; - prev = g_threadlock; if (newhandler) - g_threadlock = newhandler; + pg_g_threadlock = newhandler; else - g_threadlock = default_threadlock; + pg_g_threadlock = default_threadlock; + return prev; } diff --git a/src/interfaces/libpq/fe-print.c b/src/interfaces/libpq/fe-print.c index 9992de533d..8c2ce003ac 100644 --- a/src/interfaces/libpq/fe-print.c +++ b/src/interfaces/libpq/fe-print.c @@ -10,7 +10,7 @@ * didn't really belong there. * * IDENTIFICATION - * $PostgreSQL: pgsql/src/interfaces/libpq/fe-print.c,v 1.56 2004/12/02 15:32:54 momjian Exp $ + * $PostgreSQL: pgsql/src/interfaces/libpq/fe-print.c,v 1.57 2004/12/02 23:20:20 tgl Exp $ * *------------------------------------------------------------------------- */ @@ -193,8 +193,8 @@ PQprint(FILE *fout, { usePipe = 1; #ifdef ENABLE_THREAD_SAFETY - pq_block_sigpipe(&osigset, &sigpipe_pending); - sigpipe_masked = true; + if (pq_block_sigpipe(&osigset, &sigpipe_pending) == 0) + sigpipe_masked = true; #else #ifndef WIN32 oldsigpipehandler = pqsignal(SIGPIPE, SIG_IGN); @@ -316,8 +316,9 @@ PQprint(FILE *fout, pclose(fout); #endif #ifdef ENABLE_THREAD_SAFETY + /* we can't easily verify if EPIPE occurred, so say it did */ if (sigpipe_masked) - pq_reset_sigpipe(&osigset, sigpipe_pending); + pq_reset_sigpipe(&osigset, sigpipe_pending, true); #else #ifndef WIN32 pqsignal(SIGPIPE, oldsigpipehandler); diff --git a/src/interfaces/libpq/fe-secure.c b/src/interfaces/libpq/fe-secure.c index 79ea192f99..a40b92065b 100644 --- a/src/interfaces/libpq/fe-secure.c +++ b/src/interfaces/libpq/fe-secure.c @@ -11,7 +11,7 @@ * * * IDENTIFICATION - * $PostgreSQL: pgsql/src/interfaces/libpq/fe-secure.c,v 1.58 2004/12/02 15:32:54 momjian Exp $ + * $PostgreSQL: pgsql/src/interfaces/libpq/fe-secure.c,v 1.59 2004/12/02 23:20:21 tgl Exp $ * * NOTES * [ Most of these notes are wrong/obsolete, but perhaps not all ] @@ -147,7 +147,7 @@ static void SSLerrfree(char *buf); #endif #ifdef USE_SSL -bool pq_initssllib = true; +static bool pq_initssllib = true; static SSL_CTX *SSL_context = NULL; #endif @@ -212,6 +212,19 @@ KWbuHn491xNO25CQWMtem80uKw+pTnisBRF/454n1Jnhub144YRBoN8CAQI=\n\ /* Procedures common to all secure sessions */ /* ------------------------------------------------------------ */ + +/* + * Exported (but as yet undocumented) function to allow application to + * tell us it's already initialized OpenSSL. + */ +void +PQinitSSL(int do_init) +{ +#ifdef USE_SSL + pq_initssllib = do_init; +#endif +} + /* * Initialize global context */ @@ -377,8 +390,10 @@ pqsecure_write(PGconn *conn, const void *ptr, size_t len) #ifdef ENABLE_THREAD_SAFETY sigset_t osigmask; bool sigpipe_pending; + bool got_epipe = false; - pq_block_sigpipe(&osigmask, &sigpipe_pending); + if (pq_block_sigpipe(&osigmask, &sigpipe_pending) < 0) + return -1; #else #ifndef WIN32 pqsigfunc oldsighandler = pqsignal(SIGPIPE, SIG_IGN); @@ -413,9 +428,13 @@ pqsecure_write(PGconn *conn, const void *ptr, size_t len) char sebuf[256]; if (n == -1) + { + if (SOCK_ERRNO == EPIPE) + got_epipe = true; printfPQExpBuffer(&conn->errorMessage, libpq_gettext("SSL SYSCALL error: %s\n"), SOCK_STRERROR(SOCK_ERRNO, sebuf, sizeof(sebuf))); + } else { printfPQExpBuffer(&conn->errorMessage, @@ -448,15 +467,16 @@ pqsecure_write(PGconn *conn, const void *ptr, size_t len) } else #endif + { n = send(conn->sock, ptr, len, 0); - /* - * Possible optimization: if sigpending() turns out to be an - * expensive operation, we can set sigpipe_pending = 'true' - * here if errno != EPIPE, avoiding a sigpending call. - */ +#ifdef ENABLE_THREAD_SAFETY + if (n < 0 && SOCK_ERRNO == EPIPE) + got_epipe = true; +#endif + } #ifdef ENABLE_THREAD_SAFETY - pq_reset_sigpipe(&osigmask, sigpipe_pending); + pq_reset_sigpipe(&osigmask, sigpipe_pending, got_epipe); #else #ifndef WIN32 pqsignal(SIGPIPE, oldsighandler); @@ -1228,13 +1248,14 @@ pq_block_sigpipe(sigset_t *osigset, bool *sigpipe_pending) { sigset_t sigpipe_sigset; sigset_t sigset; - int ret; sigemptyset(&sigpipe_sigset); sigaddset(&sigpipe_sigset, SIGPIPE); /* Block SIGPIPE and save previous mask for later reset */ - ret = pthread_sigmask(SIG_BLOCK, &sigpipe_sigset, osigset); + SOCK_ERRNO_SET(pthread_sigmask(SIG_BLOCK, &sigpipe_sigset, osigset)); + if (SOCK_ERRNO) + return -1; /* We can have a pending SIGPIPE only if it was blocked before */ if (sigismember(osigset, SIGPIPE)) @@ -1251,44 +1272,52 @@ pq_block_sigpipe(sigset_t *osigset, bool *sigpipe_pending) else *sigpipe_pending = false; - return ret; + return 0; } /* * Discard any pending SIGPIPE and reset the signal mask. - * We might be discarding a blocked SIGPIPE that we didn't generate, - * but we document that you can't keep blocked SIGPIPE calls across - * libpq function calls. + * + * Note: we are effectively assuming here that the C library doesn't queue + * up multiple SIGPIPE events. If it did, then we'd accidentally leave + * ours in the queue when an event was already pending and we got another. + * As long as it doesn't queue multiple events, we're OK because the caller + * can't tell the difference. + * + * The caller should say got_epipe = FALSE if it is certain that it + * didn't get an EPIPE error; in that case we'll skip the clear operation + * and things are definitely OK, queuing or no. If it got one or might have + * gotten one, pass got_epipe = TRUE. + * + * We do not want this to change errno, since if it did that could lose + * the error code from a preceding send(). We essentially assume that if + * we were able to do pq_block_sigpipe(), this can't fail. */ -int -pq_reset_sigpipe(sigset_t *osigset, bool sigpipe_pending) +void +pq_reset_sigpipe(sigset_t *osigset, bool sigpipe_pending, bool got_epipe) { + int save_errno = SOCK_ERRNO; int signo; sigset_t sigset; /* Clear SIGPIPE only if none was pending */ - if (!sigpipe_pending) + if (got_epipe && !sigpipe_pending) { - if (sigpending(&sigset) != 0) - return -1; - - /* - * Discard pending and blocked SIGPIPE - */ - if (sigismember(&sigset, SIGPIPE)) + if (sigpending(&sigset) == 0 && + sigismember(&sigset, SIGPIPE)) { sigset_t sigpipe_sigset; sigemptyset(&sigpipe_sigset); sigaddset(&sigpipe_sigset, SIGPIPE); - + sigwait(&sigpipe_sigset, &signo); - if (signo != SIGPIPE) - return -1; } } /* Restore saved block mask */ - return pthread_sigmask(SIG_SETMASK, osigset, NULL); + pthread_sigmask(SIG_SETMASK, osigset, NULL); + + SOCK_ERRNO_SET(save_errno); } #endif diff --git a/src/interfaces/libpq/libpq-fe.h b/src/interfaces/libpq/libpq-fe.h index b472a1ae81..638c27542c 100644 --- a/src/interfaces/libpq/libpq-fe.h +++ b/src/interfaces/libpq/libpq-fe.h @@ -7,7 +7,7 @@ * Portions Copyright (c) 1996-2004, PostgreSQL Global Development Group * Portions Copyright (c) 1994, Regents of the University of California * - * $PostgreSQL: pgsql/src/interfaces/libpq/libpq-fe.h,v 1.114 2004/12/02 15:32:54 momjian Exp $ + * $PostgreSQL: pgsql/src/interfaces/libpq/libpq-fe.h,v 1.115 2004/12/02 23:20:21 tgl Exp $ * *------------------------------------------------------------------------- */ @@ -279,6 +279,9 @@ extern SSL *PQgetssl(PGconn *conn); extern void *PQgetssl(PGconn *conn); #endif +/* Tell libpq whether it needs to initialize OpenSSL */ +extern void PQinitSSL(int do_init); + /* Set verbosity for PQerrorMessage and PQresultErrorMessage */ extern PGVerbosity PQsetErrorVerbosity(PGconn *conn, PGVerbosity verbosity); @@ -301,11 +304,9 @@ extern PQnoticeProcessor PQsetNoticeProcessor(PGconn *conn, * Only required for multithreaded apps that use kerberos * both within their app and for postgresql connections. */ -typedef void (pgthreadlock_t) (int acquire); +typedef void (*pgthreadlock_t) (int acquire); -extern pgthreadlock_t *PQregisterThreadLock(pgthreadlock_t *newhandler); - -extern void PQinitSSL(int do_init); +extern pgthreadlock_t PQregisterThreadLock(pgthreadlock_t newhandler); /* === in fe-exec.c === */ @@ -495,8 +496,6 @@ extern int PQdsplen(const unsigned char *s, int encoding); /* Get encoding id from environment variable PGCLIENTENCODING */ extern int PQenv2encoding(void); -/* === in fe-secure.c === */ - #ifdef __cplusplus } #endif diff --git a/src/interfaces/libpq/libpq-int.h b/src/interfaces/libpq/libpq-int.h index 13b7e3588c..7657b364a5 100644 --- a/src/interfaces/libpq/libpq-int.h +++ b/src/interfaces/libpq/libpq-int.h @@ -12,7 +12,7 @@ * Portions Copyright (c) 1996-2004, PostgreSQL Global Development Group * Portions Copyright (c) 1994, Regents of the University of California * - * $PostgreSQL: pgsql/src/interfaces/libpq/libpq-int.h,v 1.97 2004/12/02 15:32:54 momjian Exp $ + * $PostgreSQL: pgsql/src/interfaces/libpq/libpq-int.h,v 1.98 2004/12/02 23:20:21 tgl Exp $ * *------------------------------------------------------------------------- */ @@ -379,13 +379,13 @@ extern int pqPacketSend(PGconn *conn, char pack_type, const void *buf, size_t buf_len); #ifdef ENABLE_THREAD_SAFETY -extern pgthreadlock_t *g_threadlock; +extern pgthreadlock_t pg_g_threadlock; -#define pglock_thread() g_threadlock(true); -#define pgunlock_thread() g_threadlock(false); +#define pglock_thread() pg_g_threadlock(true) +#define pgunlock_thread() pg_g_threadlock(false) #else -#define pglock_thread() ((void)0) -#define pgunlock_thread() ((void)0) +#define pglock_thread() ((void) 0) +#define pgunlock_thread() ((void) 0) #endif @@ -476,13 +476,10 @@ extern void pqsecure_close(PGconn *); extern ssize_t pqsecure_read(PGconn *, void *ptr, size_t len); extern ssize_t pqsecure_write(PGconn *, const void *ptr, size_t len); -#ifdef USE_SSL -extern bool pq_initssllib; -#endif - #ifdef ENABLE_THREAD_SAFETY -int pq_block_sigpipe(sigset_t *osigset, bool *sigpipe_pending); -int pq_reset_sigpipe(sigset_t *osigset, bool sigpipe_pending); +extern int pq_block_sigpipe(sigset_t *osigset, bool *sigpipe_pending); +extern void pq_reset_sigpipe(sigset_t *osigset, bool sigpipe_pending, + bool got_epipe); #endif /* -- 2.40.0