Skip to content

Commit 5d7a555

Browse files
tglsfdctglsfdc
committed
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.
1 parent 9b711e7 commit 5d7a555

File tree

5 files changed

+90
-71
lines changed

5 files changed

+90
-71
lines changed

‎src/interfaces/libpq/fe-connect.c

Lines changed: 12 additions & 19 deletions
Original file line numberDiff line numberDiff line change
@@ -8,7 +8,7 @@
88
*
99
*
1010
* IDENTIFICATION
11-
* $PostgreSQL: pgsql/src/interfaces/libpq/fe-connect.c,v 1.291 2004/12/02 15:32:54 momjian Exp $
11+
* $PostgreSQL: pgsql/src/interfaces/libpq/fe-connect.c,v 1.292 2004/12/02 23:20:19 tgl Exp $
1212
*
1313
*-------------------------------------------------------------------------
1414
*/
@@ -202,6 +202,11 @@ static int parseServiceInfo(PQconninfoOption *options,
202202
static char *pwdfMatchesString(char *buf, char *token);
203203
static char *PasswordFromFile(char *hostname, char *port, char *dbname,
204204
char *username);
205+
static void default_threadlock(int acquire);
206+
207+
208+
/* global variable because fe-auth.c needs to access it */
209+
pgthreadlock_t pg_g_threadlock = default_threadlock;
205210

206211

207212
/*
@@ -3276,16 +3281,6 @@ PasswordFromFile(char *hostname, char *port, char *dbname, char *username)
32763281
* if they are not required.
32773282
*/
32783283

3279-
void
3280-
PQinitSSL(int do_init)
3281-
{
3282-
#ifdef USE_SSL
3283-
pq_initssllib = do_init;
3284-
#endif
3285-
}
3286-
3287-
static pgthreadlock_t default_threadlock;
3288-
32893284
static void
32903285
default_threadlock(int acquire)
32913286
{
@@ -3313,17 +3308,15 @@ default_threadlock(int acquire)
33133308
#endif
33143309
}
33153310

3316-
pgthreadlock_t *g_threadlock = default_threadlock;
3317-
3318-
pgthreadlock_t *
3319-
PQregisterThreadLock(pgthreadlock_t *newhandler)
3311+
pgthreadlock_t
3312+
PQregisterThreadLock(pgthreadlock_t newhandler)
33203313
{
3321-
pgthreadlock_t *prev;
3314+
pgthreadlock_t prev = pg_g_threadlock;
33223315

3323-
prev = g_threadlock;
33243316
if (newhandler)
3325-
g_threadlock = newhandler;
3317+
pg_g_threadlock = newhandler;
33263318
else
3327-
g_threadlock = default_threadlock;
3319+
pg_g_threadlock = default_threadlock;
3320+
33283321
return prev;
33293322
}

‎src/interfaces/libpq/fe-print.c

Lines changed: 5 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -10,7 +10,7 @@
1010
* didn't really belong there.
1111
*
1212
* IDENTIFICATION
13-
* $PostgreSQL: pgsql/src/interfaces/libpq/fe-print.c,v 1.56 2004/12/02 15:32:54 momjian Exp $
13+
* $PostgreSQL: pgsql/src/interfaces/libpq/fe-print.c,v 1.57 2004/12/02 23:20:20 tgl Exp $
1414
*
1515
*-------------------------------------------------------------------------
1616
*/
@@ -193,8 +193,8 @@ PQprint(FILE *fout,
193193
{
194194
usePipe = 1;
195195
#ifdef ENABLE_THREAD_SAFETY
196-
pq_block_sigpipe(&osigset, &sigpipe_pending);
197-
sigpipe_masked = true;
196+
if (pq_block_sigpipe(&osigset, &sigpipe_pending) == 0)
197+
sigpipe_masked = true;
198198
#else
199199
#ifndef WIN32
200200
oldsigpipehandler = pqsignal(SIGPIPE, SIG_IGN);
@@ -316,8 +316,9 @@ PQprint(FILE *fout,
316316
pclose(fout);
317317
#endif
318318
#ifdef ENABLE_THREAD_SAFETY
319+
/* we can't easily verify if EPIPE occurred, so say it did */
319320
if (sigpipe_masked)
320-
pq_reset_sigpipe(&osigset, sigpipe_pending);
321+
pq_reset_sigpipe(&osigset, sigpipe_pending, true);
321322
#else
322323
#ifndef WIN32
323324
pqsignal(SIGPIPE, oldsigpipehandler);

‎src/interfaces/libpq/fe-secure.c

Lines changed: 58 additions & 29 deletions
Original file line numberDiff line numberDiff line change
@@ -11,7 +11,7 @@
1111
*
1212
*
1313
* IDENTIFICATION
14-
* $PostgreSQL: pgsql/src/interfaces/libpq/fe-secure.c,v 1.58 2004/12/02 15:32:54 momjian Exp $
14+
* $PostgreSQL: pgsql/src/interfaces/libpq/fe-secure.c,v 1.59 2004/12/02 23:20:21 tgl Exp $
1515
*
1616
* NOTES
1717
* [ Most of these notes are wrong/obsolete, but perhaps not all ]
@@ -147,7 +147,7 @@ static void SSLerrfree(char *buf);
147147
#endif
148148

149149
#ifdef USE_SSL
150-
bool pq_initssllib = true;
150+
static bool pq_initssllib = true;
151151

152152
static SSL_CTX *SSL_context = NULL;
153153
#endif
@@ -212,6 +212,19 @@ KWbuHn491xNO25CQWMtem80uKw+pTnisBRF/454n1Jnhub144YRBoN8CAQI=\n\
212212
/* Procedures common to all secure sessions */
213213
/* ------------------------------------------------------------ */
214214

215+
216+
/*
217+
* Exported (but as yet undocumented) function to allow application to
218+
* tell us it's already initialized OpenSSL.
219+
*/
220+
void
221+
PQinitSSL(int do_init)
222+
{
223+
#ifdef USE_SSL
224+
pq_initssllib = do_init;
225+
#endif
226+
}
227+
215228
/*
216229
* Initialize global context
217230
*/
@@ -377,8 +390,10 @@ pqsecure_write(PGconn *conn, const void *ptr, size_t len)
377390
#ifdef ENABLE_THREAD_SAFETY
378391
sigset_t osigmask;
379392
bool sigpipe_pending;
393+
bool got_epipe = false;
380394

381-
pq_block_sigpipe(&osigmask, &sigpipe_pending);
395+
if (pq_block_sigpipe(&osigmask, &sigpipe_pending) < 0)
396+
return -1;
382397
#else
383398
#ifndef WIN32
384399
pqsigfunc oldsighandler = pqsignal(SIGPIPE, SIG_IGN);
@@ -413,9 +428,13 @@ pqsecure_write(PGconn *conn, const void *ptr, size_t len)
413428
char sebuf[256];
414429

415430
if (n == -1)
431+
{
432+
if (SOCK_ERRNO == EPIPE)
433+
got_epipe = true;
416434
printfPQExpBuffer(&conn->errorMessage,
417435
libpq_gettext("SSL SYSCALL error: %s\n"),
418436
SOCK_STRERROR(SOCK_ERRNO, sebuf, sizeof(sebuf)));
437+
}
419438
else
420439
{
421440
printfPQExpBuffer(&conn->errorMessage,
@@ -448,15 +467,16 @@ pqsecure_write(PGconn *conn, const void *ptr, size_t len)
448467
}
449468
else
450469
#endif
470+
{
451471
n = send(conn->sock, ptr, len, 0);
452-
/*
453-
* Possible optimization: if sigpending() turns out to be an
454-
* expensive operation, we can set sigpipe_pending = 'true'
455-
* here if errno != EPIPE, avoiding a sigpending call.
456-
*/
472+
#ifdef ENABLE_THREAD_SAFETY
473+
if (n < 0 && SOCK_ERRNO == EPIPE)
474+
got_epipe = true;
475+
#endif
476+
}
457477

458478
#ifdef ENABLE_THREAD_SAFETY
459-
pq_reset_sigpipe(&osigmask, sigpipe_pending);
479+
pq_reset_sigpipe(&osigmask, sigpipe_pending, got_epipe);
460480
#else
461481
#ifndef WIN32
462482
pqsignal(SIGPIPE, oldsighandler);
@@ -1228,13 +1248,14 @@ pq_block_sigpipe(sigset_t *osigset, bool *sigpipe_pending)
12281248
{
12291249
sigset_t sigpipe_sigset;
12301250
sigset_t sigset;
1231-
int ret;
12321251

12331252
sigemptyset(&sigpipe_sigset);
12341253
sigaddset(&sigpipe_sigset, SIGPIPE);
12351254

12361255
/* Block SIGPIPE and save previous mask for later reset */
1237-
ret = pthread_sigmask(SIG_BLOCK, &sigpipe_sigset, osigset);
1256+
SOCK_ERRNO_SET(pthread_sigmask(SIG_BLOCK, &sigpipe_sigset, osigset));
1257+
if (SOCK_ERRNO)
1258+
return -1;
12381259

12391260
/* We can have a pending SIGPIPE only if it was blocked before */
12401261
if (sigismember(osigset, SIGPIPE))
@@ -1251,44 +1272,52 @@ pq_block_sigpipe(sigset_t *osigset, bool *sigpipe_pending)
12511272
else
12521273
*sigpipe_pending = false;
12531274

1254-
return ret;
1275+
return 0;
12551276
}
12561277

12571278
/*
12581279
* Discard any pending SIGPIPE and reset the signal mask.
1259-
* We might be discarding a blocked SIGPIPE that we didn't generate,
1260-
* but we document that you can't keep blocked SIGPIPE calls across
1261-
* libpq function calls.
1280+
*
1281+
* Note: we are effectively assuming here that the C library doesn't queue
1282+
* up multiple SIGPIPE events. If it did, then we'd accidentally leave
1283+
* ours in the queue when an event was already pending and we got another.
1284+
* As long as it doesn't queue multiple events, we're OK because the caller
1285+
* can't tell the difference.
1286+
*
1287+
* The caller should say got_epipe = FALSE if it is certain that it
1288+
* didn't get an EPIPE error; in that case we'll skip the clear operation
1289+
* and things are definitely OK, queuing or no. If it got one or might have
1290+
* gotten one, pass got_epipe = TRUE.
1291+
*
1292+
* We do not want this to change errno, since if it did that could lose
1293+
* the error code from a preceding send(). We essentially assume that if
1294+
* we were able to do pq_block_sigpipe(), this can't fail.
12621295
*/
1263-
int
1264-
pq_reset_sigpipe(sigset_t *osigset, bool sigpipe_pending)
1296+
void
1297+
pq_reset_sigpipe(sigset_t *osigset, bool sigpipe_pending, bool got_epipe)
12651298
{
1299+
int save_errno = SOCK_ERRNO;
12661300
int signo;
12671301
sigset_t sigset;
12681302

12691303
/* Clear SIGPIPE only if none was pending */
1270-
if (!sigpipe_pending)
1304+
if (got_epipe && !sigpipe_pending)
12711305
{
1272-
if (sigpending(&sigset) != 0)
1273-
return -1;
1274-
1275-
/*
1276-
* Discard pending and blocked SIGPIPE
1277-
*/
1278-
if (sigismember(&sigset, SIGPIPE))
1306+
if (sigpending(&sigset) == 0 &&
1307+
sigismember(&sigset, SIGPIPE))
12791308
{
12801309
sigset_t sigpipe_sigset;
12811310

12821311
sigemptyset(&sigpipe_sigset);
12831312
sigaddset(&sigpipe_sigset, SIGPIPE);
1284-
1313+
12851314
sigwait(&sigpipe_sigset, &signo);
1286-
if (signo != SIGPIPE)
1287-
return -1;
12881315
}
12891316
}
12901317

12911318
/* Restore saved block mask */
1292-
return pthread_sigmask(SIG_SETMASK, osigset, NULL);
1319+
pthread_sigmask(SIG_SETMASK, osigset, NULL);
1320+
1321+
SOCK_ERRNO_SET(save_errno);
12931322
}
12941323
#endif

‎src/interfaces/libpq/libpq-fe.h

Lines changed: 6 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -7,7 +7,7 @@
77
* Portions Copyright (c) 1996-2004, PostgreSQL Global Development Group
88
* Portions Copyright (c) 1994, Regents of the University of California
99
*
10-
* $PostgreSQL: pgsql/src/interfaces/libpq/libpq-fe.h,v 1.114 2004/12/02 15:32:54 momjian Exp $
10+
* $PostgreSQL: pgsql/src/interfaces/libpq/libpq-fe.h,v 1.115 2004/12/02 23:20:21 tgl Exp $
1111
*
1212
*-------------------------------------------------------------------------
1313
*/
@@ -279,6 +279,9 @@ extern SSL *PQgetssl(PGconn *conn);
279279
extern void *PQgetssl(PGconn *conn);
280280
#endif
281281

282+
/* Tell libpq whether it needs to initialize OpenSSL */
283+
extern void PQinitSSL(int do_init);
284+
282285
/* Set verbosity for PQerrorMessage and PQresultErrorMessage */
283286
extern PGVerbosity PQsetErrorVerbosity(PGconn *conn, PGVerbosity verbosity);
284287

@@ -301,11 +304,9 @@ extern PQnoticeProcessor PQsetNoticeProcessor(PGconn *conn,
301304
* Only required for multithreaded apps that use kerberos
302305
* both within their app and for postgresql connections.
303306
*/
304-
typedef void (pgthreadlock_t) (int acquire);
307+
typedef void (*pgthreadlock_t) (int acquire);
305308

306-
extern pgthreadlock_t *PQregisterThreadLock(pgthreadlock_t *newhandler);
307-
308-
extern void PQinitSSL(int do_init);
309+
extern pgthreadlock_t PQregisterThreadLock(pgthreadlock_t newhandler);
309310

310311
/* === in fe-exec.c === */
311312

@@ -495,8 +496,6 @@ extern int PQdsplen(const unsigned char *s, int encoding);
495496
/* Get encoding id from environment variable PGCLIENTENCODING */
496497
extern int PQenv2encoding(void);
497498

498-
/* === in fe-secure.c === */
499-
500499
#ifdef __cplusplus
501500
}
502501
#endif

‎src/interfaces/libpq/libpq-int.h

Lines changed: 9 additions & 12 deletions
Original file line numberDiff line numberDiff line change
@@ -12,7 +12,7 @@
1212
* Portions Copyright (c) 1996-2004, PostgreSQL Global Development Group
1313
* Portions Copyright (c) 1994, Regents of the University of California
1414
*
15-
* $PostgreSQL: pgsql/src/interfaces/libpq/libpq-int.h,v 1.97 2004/12/02 15:32:54 momjian Exp $
15+
* $PostgreSQL: pgsql/src/interfaces/libpq/libpq-int.h,v 1.98 2004/12/02 23:20:21 tgl Exp $
1616
*
1717
*-------------------------------------------------------------------------
1818
*/
@@ -379,13 +379,13 @@ extern int pqPacketSend(PGconn *conn, char pack_type,
379379
const void *buf, size_t buf_len);
380380

381381
#ifdef ENABLE_THREAD_SAFETY
382-
extern pgthreadlock_t *g_threadlock;
382+
extern pgthreadlock_t pg_g_threadlock;
383383

384-
#define pglock_thread() g_threadlock(true);
385-
#define pgunlock_thread() g_threadlock(false);
384+
#define pglock_thread() pg_g_threadlock(true)
385+
#define pgunlock_thread() pg_g_threadlock(false)
386386
#else
387-
#define pglock_thread() ((void)0)
388-
#define pgunlock_thread() ((void)0)
387+
#define pglock_thread() ((void) 0)
388+
#define pgunlock_thread() ((void) 0)
389389
#endif
390390

391391

@@ -476,13 +476,10 @@ extern void pqsecure_close(PGconn *);
476476
extern ssize_t pqsecure_read(PGconn *, void *ptr, size_t len);
477477
extern ssize_t pqsecure_write(PGconn *, const void *ptr, size_t len);
478478

479-
#ifdef USE_SSL
480-
extern bool pq_initssllib;
481-
#endif
482-
483479
#ifdef ENABLE_THREAD_SAFETY
484-
int pq_block_sigpipe(sigset_t *osigset, bool *sigpipe_pending);
485-
int pq_reset_sigpipe(sigset_t *osigset, bool sigpipe_pending);
480+
extern int pq_block_sigpipe(sigset_t *osigset, bool *sigpipe_pending);
481+
extern void pq_reset_sigpipe(sigset_t *osigset, bool sigpipe_pending,
482+
bool got_epipe);
486483
#endif
487484

488485
/*

0 commit comments

Comments
 (0)