[PATCH 0/2 v3] SIGPIPE masking in local socket connections

Currently, the sigpipe-masking code in libpq is implemented as
a set of macros, which depend on declaring local variables.

This change adds a (private) struct sigpipe_info to contain the
compile-dependent data required for sigpipe masking and restoring.
The caller can then declare a struct sigpipe info explicitly, and
pass this to the subsequent sigpipe-masking code.

This allows us to separate the variable declarations from the code,
and gives the caller more flexibility for controlling the scope of
these variables.

Also, since we don't need to declare variables in the macros, we
can change the code to be implemented as static inlines.

Signed-off-by: Jeremy Kerr <jk(at)ozlabs(dot)org>

---
src/interfaces/libpq/fe-secure.c | 88 ++++++++++++++++++++++++---------------
1 file changed, 55 insertions(+), 33 deletions(-)

*** a/src/interfaces/libpq/fe-secure.c
--- b/src/interfaces/libpq/fe-secure.c
***************
*** 119,163 **** static long win32_ssl_create_mutex = 0;

/*
* Macros to handle disabling and then restoring the state of SIGPIPE handling.
- * Note that DISABLE_SIGPIPE() must appear at the start of a block.
*/

#ifndef WIN32
#ifdef ENABLE_THREAD_SAFETY

! #define DISABLE_SIGPIPE(failaction) \
! sigset_t osigmask; \
! bool sigpipe_pending; \
! bool got_epipe = false; \
! \
! if (pq_block_sigpipe(&osigmask, &sigpipe_pending) < 0) \
! failaction

! #define REMEMBER_EPIPE(cond) \
! do { \
! if (cond) \
! got_epipe = true; \
! } while (0)

! #define RESTORE_SIGPIPE() \
! pq_reset_sigpipe(&osigmask, sigpipe_pending, got_epipe)

#else /* !ENABLE_THREAD_SAFETY */

! #define DISABLE_SIGPIPE(failaction) \
! pqsigfunc oldsighandler = pqsignal(SIGPIPE, SIG_IGN)

! #define REMEMBER_EPIPE(cond)

! #define RESTORE_SIGPIPE() \
! pqsignal(SIGPIPE, oldsighandler)

#endif /* ENABLE_THREAD_SAFETY */
#else /* WIN32 */

! #define DISABLE_SIGPIPE(failaction)
! #define REMEMBER_EPIPE(cond)
! #define RESTORE_SIGPIPE()

#endif /* WIN32 */

--- 119,180 ----

/*
* Macros to handle disabling and then restoring the state of SIGPIPE handling.
*/

#ifndef WIN32
#ifdef ENABLE_THREAD_SAFETY

! struct sigpipe_info {
! sigset_t oldsigmask;
! bool sigpipe_pending;
! bool got_epipe;
! };

! static inline int disable_sigpipe(struct sigpipe_info *info)
! {
! info->got_epipe = false;
! return pq_block_sigpipe(&info->oldsigmask, &info->sigpipe_pending) < 0;
! }

! static inline void remember_epipe(struct sigpipe_info *info, bool cond)
! {
! if (cond)
! info->got_epipe = true;
! }
!
! static inline void restore_sigpipe(struct sigpipe_info *info)
! {
! pq_reset_sigpipe(&info->oldsigmask, info->sigpipe_pending, info->got_epipe);
! }

#else /* !ENABLE_THREAD_SAFETY */

! struct sigpipe_info {
! pqsigfunc oldhandler;
! };

! static inline int disable_sigpipe(struct sigpipe_info *info)
! {
! info->oldhandler = pqsignal(SIGPIPE, SIG_IGN);
! return 0;
! }
!
! static inline void remember_epipe(struct sigpipe_info *info, bool cond)
! {
! }

! static inline void restore_sigpipe(struct sigpipe_info *info)
! {
! pqsignal(SIGPIPE, info->oldhandler);
! }

#endif /* ENABLE_THREAD_SAFETY */
#else /* WIN32 */

! struct sigpipe_info { };
! static inline int disable_sigpipe(struct sigpipe_info *info) { return 0; }
! static inline void remember_epipe(struct sigpipe_info *info, bool cond) { }
! static inline void restore_sigpipe(struct sigpipe_info *info) { }

#endif /* WIN32 */

***************
*** 286,294 **** pqsecure_read(PGconn *conn, void *ptr, size_t len)
if (conn->ssl)
{
int err;

/* SSL_read can write to the socket, so we need to disable SIGPIPE */
! DISABLE_SIGPIPE(return -1);

rloop:
n = SSL_read(conn->ssl, ptr, len);
--- 303,313 ----
if (conn->ssl)
{
int err;
+ struct sigpipe_info info;

/* SSL_read can write to the socket, so we need to disable SIGPIPE */
! if (disable_sigpipe(&info))
! return -1;

rloop:
n = SSL_read(conn->ssl, ptr, len);
***************
*** 315,321 **** rloop:

if (n == -1)
{
! REMEMBER_EPIPE(SOCK_ERRNO == EPIPE);
printfPQExpBuffer(&conn->errorMessage,
libpq_gettext("SSL SYSCALL error: %s\n"),
SOCK_STRERROR(SOCK_ERRNO, sebuf, sizeof(sebuf)));
--- 334,340 ----

if (n == -1)
{
! remember_epipe(&info, SOCK_ERRNO == EPIPE);
printfPQExpBuffer(&conn->errorMessage,
libpq_gettext("SSL SYSCALL error: %s\n"),
SOCK_STRERROR(SOCK_ERRNO, sebuf, sizeof(sebuf)));
***************
*** 351,357 **** rloop:
break;
}

! RESTORE_SIGPIPE();
}
else
#endif
--- 370,376 ----
break;
}

! restore_sigpipe(&info);
}
else
#endif
***************
*** 367,374 **** ssize_t
pqsecure_write(PGconn *conn, const void *ptr, size_t len)
{
ssize_t n;

! DISABLE_SIGPIPE(return -1);

#ifdef USE_SSL
if (conn->ssl)
--- 386,395 ----
pqsecure_write(PGconn *conn, const void *ptr, size_t len)
{
ssize_t n;
+ struct sigpipe_info info;

! if (disable_sigpipe(&info))
! return -1;

#ifdef USE_SSL
if (conn->ssl)
***************
*** 399,405 **** pqsecure_write(PGconn *conn, const void *ptr, size_t len)

if (n == -1)
{
! REMEMBER_EPIPE(SOCK_ERRNO == EPIPE);
printfPQExpBuffer(&conn->errorMessage,
libpq_gettext("SSL SYSCALL error: %s\n"),
SOCK_STRERROR(SOCK_ERRNO, sebuf, sizeof(sebuf)));
--- 420,426 ----

if (n == -1)
{
! remember_epipe(&info, SOCK_ERRNO == EPIPE);
printfPQExpBuffer(&conn->errorMessage,
libpq_gettext("SSL SYSCALL error: %s\n"),
SOCK_STRERROR(SOCK_ERRNO, sebuf, sizeof(sebuf)));
***************
*** 438,447 **** pqsecure_write(PGconn *conn, const void *ptr, size_t len)
#endif
{
n = send(conn->sock, ptr, len, 0);
! REMEMBER_EPIPE(n < 0 && SOCK_ERRNO == EPIPE);
}

! RESTORE_SIGPIPE();

return n;
}
--- 459,468 ----
#endif
{
n = send(conn->sock, ptr, len, 0);
! remember_epipe(&info, n < 0 && SOCK_ERRNO == EPIPE);
}

! restore_sigpipe(&info);

return n;
}
***************
*** 1197,1210 **** close_SSL(PGconn *conn)
{
if (conn->ssl)
{
! DISABLE_SIGPIPE((void) 0);
SSL_shutdown(conn->ssl);
SSL_free(conn->ssl);
conn->ssl = NULL;
pqsecure_destroy();
/* We have to assume we got EPIPE */
! REMEMBER_EPIPE(true);
! RESTORE_SIGPIPE();
}

if (conn->peer)
--- 1218,1232 ----
{
if (conn->ssl)
{
! struct sigpipe_info info;
! disable_sigpipe(&info);
SSL_shutdown(conn->ssl);
SSL_free(conn->ssl);
conn->ssl = NULL;
pqsecure_destroy();
/* We have to assume we got EPIPE */
! remember_epipe(&info, true);
! restore_sigpipe(&info);
}

if (conn->peer)

A new approach to avioding manipulating the signal mask during for every
send - this time round, use SO_NOSIGPIPE and MSG_NOSIGNAL if available.

The patches have been tested on Linux and OSX, and I've confirmed that
'struct foo { };' will compile with a MSVC compiler. I'd still like a
little more testing though, is there a machine that allows both
SO_NOSIGPIPE and MSG_NOSIGNAL?

v3: respin as context diffs

Again, comments most welcome,

Jeremy

---
Jeremy Kerr (2):
[libpq] rework sigpipe-handling macros
[libpq] Try to avoid manually masking SIGPIPEs on every send()

Currently, libpq will wrap each send() call on the connection with
two system calls to mask SIGPIPEs. This results in 3 syscalls instead
of one, and (on Linux) can lead to high contention on the signal
mask locks in threaded apps.

We have a couple of other methods to avoid SIGPIPEs:
sockopt(SO_NOSIGPIPE) and the MSG_NOSIGNAL flag to send().

This change attempts to use these if they're available at compile-
and run-time. If not, we drop back to manipulating the signal mask as
before.

Signed-off-by: Jeremy Kerr <jk(at)ozlabs(dot)org>

---
src/interfaces/libpq/fe-connect.c | 40 ++++++++++++++++++
src/interfaces/libpq/fe-secure.c | 83 +++++++++++++++++++++++++++++---------
src/interfaces/libpq/libpq-int.h | 2
3 files changed, 107 insertions(+), 18 deletions(-)

*** a/src/interfaces/libpq/fe-connect.c
--- b/src/interfaces/libpq/fe-connect.c
***************
*** 1085,1090 **** keep_going: /* We will come back to here until there is
--- 1085,1091 ----
while (conn->addr_cur != NULL)
{
struct addrinfo *addr_cur = conn->addr_cur;
+ int optval;

/* Remember current address for possible error msg */
memcpy(&conn->raddr.addr, addr_cur->ai_addr,
***************
*** 1149,1154 **** keep_going: /* We will come back to here until there is
--- 1150,1194 ----
}
#endif /* F_SETFD */

+ /* We have three methods of blocking sigpipe during
+ * send() calls to this socket:
+ *
+ * - setsockopt(sock, SO_NOSIGPIPE)
+ * - send(sock, ..., MSG_NOSIGNAL)
+ * - setting the signal mask to SIG_IGN during send()
+ *
+ * The first two reduce the number of syscalls (for the
+ * third, we require three syscalls to implement a send()),
+ * so use them if they're available. Their availability is
+ * flagged in the following members of PGconn:
+ *
+ * conn->sigpipe_so - we have set up SO_NOSIGPIPE
+ * conn->sigpipe_flag - we're specifying MSG_NOSIGNAL
+ *
+ * If we can use SO_NOSIGPIPE, then set sigpipe_so here and
+ * we don't need to care about anything else. Otherwise,
+ * try MSG_NOSIGNAL by setting sigpipe_flag. If we get an
+ * error with MSG_NOSIGNAL, we clear the flag and revert
+ * to manual masking.
+ */
+ conn->sigpipe_so = false;
+ #ifdef MSG_NOSIGNAL
+ conn->sigpipe_flag = true;
+ #else /* !MSG_NOSIGNAL */
+ conn->sigpipe_flag = false;
+ #endif /* MSG_NOSIGNAL */
+
+ #ifdef SO_NOSIGPIPE
+ optval = 1;
+ if (!setsockopt(conn->sock, SOL_SOCKET, SO_NOSIGPIPE,
+ (char *)&optval, sizeof(optval)))
+ {
+ conn->sigpipe_so = true;
+ conn->sigpipe_flag = false;
+ }
+ #endif /* SO_NOSIGPIPE */
+
+
/*
* Start/make connection. This should not block, since we
* are in nonblock mode. If it does, well, too bad.
*** a/src/interfaces/libpq/fe-secure.c
--- b/src/interfaces/libpq/fe-secure.c
***************
*** 122,127 **** static long win32_ssl_create_mutex = 0;
--- 122,139 ----
*/

#ifndef WIN32
+
+ static inline int sigpipe_masked(PGconn *conn)
+ {
+ /* If we're on an SSL connection, we can only use SO_NOSIGPIPE masking.
+ * Otherwise, we can handle SO_NOSIGPIPE or the MSG_NOSIGNAL flag */
+ #ifdef USE_SSL
+ if (conn->ssl)
+ return conn->sigpipe_so;
+ #endif
+ return conn->sigpipe_so || conn->sigpipe_flag;
+ }
+
#ifdef ENABLE_THREAD_SAFETY

struct sigpipe_info {
***************
*** 130,137 **** struct sigpipe_info {
bool got_epipe;
};

! static inline int disable_sigpipe(struct sigpipe_info *info)
{
info->got_epipe = false;
return pq_block_sigpipe(&info->oldsigmask, &info->sigpipe_pending) < 0;
}
--- 142,152 ----
bool got_epipe;
};

! static inline int disable_sigpipe(PGconn *conn, struct sigpipe_info *info)
{
+ if (sigpipe_masked(conn))
+ return 0;
+
info->got_epipe = false;
return pq_block_sigpipe(&info->oldsigmask, &info->sigpipe_pending) < 0;
}
***************
*** 142,149 **** static inline void remember_epipe(struct sigpipe_info *info, bool cond)
info->got_epipe = true;
}

! static inline void restore_sigpipe(struct sigpipe_info *info)
{
pq_reset_sigpipe(&info->oldsigmask, info->sigpipe_pending, info->got_epipe);
}

--- 157,167 ----
info->got_epipe = true;
}

! static inline void restore_sigpipe(PGconn *conn, struct sigpipe_info *info)
{
+ if (sigpipe_masked(conn))
+ return;
+
pq_reset_sigpipe(&info->oldsigmask, info->sigpipe_pending, info->got_epipe);
}

***************
*** 153,161 **** struct sigpipe_info {
pqsigfunc oldhandler;
};

! static inline int disable_sigpipe(struct sigpipe_info *info)
{
! info->oldhandler = pqsignal(SIGPIPE, SIG_IGN);
return 0;
}

--- 171,180 ----
pqsigfunc oldhandler;
};

! static inline int disable_sigpipe(PGconn *conn, struct sigpipe_info *info)
{
! if (!sigpipe_masked(conn))
! info->oldhandler = pqsignal(SIGPIPE, SIG_IGN);
return 0;
}

***************
*** 163,180 **** static inline void remember_epipe(struct sigpipe_info *info, bool cond)
{
}

! static inline void restore_sigpipe(struct sigpipe_info *info)
{
! pqsignal(SIGPIPE, info->oldhandler);
}

#endif /* ENABLE_THREAD_SAFETY */
#else /* WIN32 */

struct sigpipe_info { };
! static inline int disable_sigpipe(struct sigpipe_info *info) { return 0; }
static inline void remember_epipe(struct sigpipe_info *info, bool cond) { }
! static inline void restore_sigpipe(struct sigpipe_info *info) { }

#endif /* WIN32 */

--- 182,203 ----
{
}

! static inline void restore_sigpipe(PGconn *conn, struct sigpipe_info *info)
{
! if (!sigpipe_masked(conn))
! pqsignal(SIGPIPE, info->oldhandler);
}

#endif /* ENABLE_THREAD_SAFETY */
#else /* WIN32 */

struct sigpipe_info { };
! static inline int disable_sigpipe(PGConn *conn, struct sigpipe_info *info)
! {
! return 0;
! }
static inline void remember_epipe(struct sigpipe_info *info, bool cond) { }
! static inline void restore_sigpipe(PGConn *conn, struct sigpipe_info *info) { }

#endif /* WIN32 */

***************
*** 306,312 **** pqsecure_read(PGconn *conn, void *ptr, size_t len)
struct sigpipe_info info;

/* SSL_read can write to the socket, so we need to disable SIGPIPE */
! if (disable_sigpipe(&info))
return -1;

rloop:
--- 329,335 ----
struct sigpipe_info info;

/* SSL_read can write to the socket, so we need to disable SIGPIPE */
! if (disable_sigpipe(conn, &info))
return -1;

rloop:
***************
*** 370,376 **** rloop:
break;
}

! restore_sigpipe(&info);
}
else
#endif
--- 393,399 ----
break;
}

! restore_sigpipe(conn, &info);
}
else
#endif
***************
*** 388,401 **** pqsecure_write(PGconn *conn, const void *ptr, size_t len)
ssize_t n;
struct sigpipe_info info;

- if (disable_sigpipe(&info))
- return -1;
-
#ifdef USE_SSL
if (conn->ssl)
{
int err;

n = SSL_write(conn->ssl, ptr, len);
err = SSL_get_error(conn->ssl, n);
switch (err)
--- 411,424 ----
ssize_t n;
struct sigpipe_info info;

#ifdef USE_SSL
if (conn->ssl)
{
int err;

+ if (disable_sigpipe(conn, &info))
+ return -1;
+
n = SSL_write(conn->ssl, ptr, len);
err = SSL_get_error(conn->ssl, n);
switch (err)
***************
*** 458,468 **** pqsecure_write(PGconn *conn, const void *ptr, size_t len)
else
#endif
{
! n = send(conn->sock, ptr, len, 0);
! remember_epipe(&info, n < 0 && SOCK_ERRNO == EPIPE);
}

! restore_sigpipe(&info);

return n;
}
--- 481,515 ----
else
#endif
{
! int flags = 0;
!
! #ifdef MSG_NOSIGNAL
! if (!conn->sigpipe_so && conn->sigpipe_flag)
! flags |= MSG_NOSIGNAL;
! #endif /* MSG_NOSIGNAL */
!
! retry_masked:
! if (disable_sigpipe(conn, &info))
! return -1;
!
! n = send(conn->sock, ptr, len, flags);
!
! if (n < 0) {
! /* if we see an EINVAL, it may be because MSG_NOSIGNAL isn't
! * available on this machine. So, clear sigpipe_flag so we don't
! * try this flag again, and retry the send().
! */
! if (flags != 0 && SOCK_ERRNO == EINVAL) {
! conn->sigpipe_flag = false;
! flags = 0;
! goto retry_masked;
! }
!
! remember_epipe(&info, SOCK_ERRNO == EPIPE);
! }
}

! restore_sigpipe(conn, &info);

return n;
}
***************
*** 1219,1232 **** close_SSL(PGconn *conn)
if (conn->ssl)
{
struct sigpipe_info info;
! disable_sigpipe(&info);
SSL_shutdown(conn->ssl);
SSL_free(conn->ssl);
conn->ssl = NULL;
pqsecure_destroy();
/* We have to assume we got EPIPE */
remember_epipe(&info, true);
! restore_sigpipe(&info);
}

if (conn->peer)
--- 1266,1279 ----
if (conn->ssl)
{
struct sigpipe_info info;
! disable_sigpipe(conn, &info);
SSL_shutdown(conn->ssl);
SSL_free(conn->ssl);
conn->ssl = NULL;
pqsecure_destroy();
/* We have to assume we got EPIPE */
remember_epipe(&info, true);
! restore_sigpipe(conn, &info);
}

if (conn->peer)
*** a/src/interfaces/libpq/libpq-int.h
--- b/src/interfaces/libpq/libpq-int.h
***************
*** 336,341 **** struct pg_conn
--- 336,343 ----
ProtocolVersion pversion; /* FE/BE protocol version in use */
int sversion; /* server version, e.g. 70401 for 7.4.1 */
bool password_needed; /* true if server demanded a password */
+ bool sigpipe_so; /* have we masked sigpipes via SO_NOSIGPIPE? */
+ bool sigpipe_flag; /* can we mask sigpipes via MSG_NOSIGNAL? */

/* Transient state needed while establishing connection */
struct addrinfo *addrlist; /* list of possible backend addresses */

Jeremy Kerr wrote:

> Also, since we don't need to declare variables in the macros, we
> can change the code to be implemented as static inlines.

Does this work in compilers other than GCC? I think we use some kludges
to protect against them ... see pg_list.h for the canonical example.

--
Alvaro Herrera http://www.CommandPrompt.com/
PostgreSQL Replication, Consulting, Custom Development, 24x7 support

Hi, Alvaro,

> Does this work in compilers other than GCC? I think we use some
> kludges to protect against them ... see pg_list.h for the canonical
> example.

As I understand it, we're not using static inlines in pg_list.h to
prevent multiple objects from exporting the same symbols if the
functions don't end up as 'static inline' (ie, because the compiler
doesn't support that).

In this case, we're only compiling the inlines into a single object, so
even if the compiler doesn't support inlines, we'll just end up with
out-of-line function calls, which should work too.

However, this is only my assumption about those compilers (I don't have
access to other compilers to test); happy to fix these up if the inlines
won't work.

Cheers,

Jeremy

On Tue, Jul 14, 2009 at 8:42 PM, Jeremy Kerr<jk(at)ozlabs(dot)org> wrote:
> Hi, Alvaro,
>
>> Does this work in compilers other than GCC?  I think we use some
>> kludges to protect against them ... see pg_list.h for the canonical
>> example.
>
> As I understand it, we're not using static inlines in pg_list.h to
> prevent multiple objects from exporting the same symbols if the
> functions don't end up as 'static inline' (ie, because the compiler
> doesn't support that).
>
> In this case, we're only compiling the inlines into a single object, so
> even if the compiler doesn't support inlines, we'll just end up with
> out-of-line function calls, which should work too.

Perhaps we should use macros.

...Robert

Hi Robert,

> Perhaps we should use macros.

I was trying to avoid macros, as this means we lose type- and syntax-
checking at the call-site, and end up with slightly messier code.
However, I understand that this is probably personal preference for me
:)

How about just 'static' functions? (ie, drop the 'inline'). This way,
the compiler is free to inline where suitable, and non-inlining
compilers will do the right thing too.

However, I'd rather make decisions on data, rather than guessing. Is the
actual problem here that some compilers just don't support the 'inline'
keyword?

Cheers,

Jeremy

Jeremy Kerr <jk(at)ozlabs(dot)org> writes:
> However, I'd rather make decisions on data, rather than guessing. Is the
> actual problem here that some compilers just don't support the 'inline'
> keyword?

I think Alvaro's complaint is unfounded --- we already have logic
to #define inline as empty if the compiler doesn't support it.
The issue he's thinking of is that non-gcc compilers typically don't
react very well to static function definitions in .h files. However
that doesn't apply to the proposed usage, since they're not going to
be in a .h file.

However, I think the whole patch is pretty useless. That code is not
broken as it stands, and doesn't appear to really gain anything from the
proposed change. Why should we risk any portability questions when the
code isn't going to get either simpler or shorter?

regards, tom lane

Tom,

> However, I think the whole patch is pretty useless. That code is not
> broken as it stands, and doesn't appear to really gain anything from
> the proposed change. Why should we risk any portability questions
> when the code isn't going to get either simpler or shorter?

This patch "clears the way" for the proceeding change (2/2). We use the
new inline functions to implement the proper checks to see if the
sigpipe-masking syscalls are needed.

We also need disable_sigpipe to be called when it's not the start of a
block, hence the separate type definition.

Cheers,

Jeremy

Jeremy Kerr <jk(at)ozlabs(dot)org> writes:
> We also need disable_sigpipe to be called when it's not the start of a
> block, hence the separate type definition.

So break that macro into two (variable definition and code).

regards, tom lane

Hi Tom,

> That code is not broken as it stands, and doesn't appear to really
> gain anything from the proposed change. Why should we risk any
> portability questions when the code isn't going to get either simpler
> or shorter?

OK, after attempting a macro version of this, we end up with:

#define DECLARE_SIGPIPE_INFO(var) struct sigpipe_info var;

#define DISABLE_SIGPIPE(info, conn) \
((SIGPIPE_MASKED(conn)) ? 0 : \
((info)->got_epipe = false, \
pg_block_sigpipe(&(info)->oldsigmask, &(info)->sigpipe_pending)) < 0)

- kinda ugly, uses the ?: and , operators to return the result of
pg_block_sigpipe. We could avoid the comma with a block though.

If we want to keep the current 'failaction' style of macro:

#define DISABLE_SIGPIPE(info, conn, failaction) \
do { \
if (!SIGPIPE_MASKED(conn)) { \
(info)->got_epipe = false; \
if (pg_block_sigpipe(&(info)->oldsigmask, \
&(info)->sigpipe_pending)) < 0) { \
failaction; \
} \
} \
} while (0)

We could ditch struct sigpipe info, but that means we need to declare
three separate vars in DECLARE_SIGPIPE_INFO() instead of the one, and it
doesn't clean up the macro code much. I'd rather reduce the amount of
stuff that we declare "behind the caller's back".

Compared to the static-function version:

static inline int disable_sigpipe(PGconn *conn, struct sigpipe_info
*info)
{
if (sigpipe_masked(conn))
return 0;

info->got_epipe = false;
return pq_block_sigpipe(&info->oldsigmask, &info->sigpipe_pending) < 0;
}

Personally, I think the static functions are a lot cleaner, and don't
think we lose any portability from using these (since inline is #define-
ed out on compilers that don't support it). On non-inlining compilers,
we do gain an extra function call, but we're about to enter the kernel
anyway, so this will probably be lost in the noise (especially if we
save the sigpipe-masking syscalls).

But in the end, it's not up to me - do you still prefer the macro
approach?

Cheers,

Jeremy

On Mon, Jul 20, 2009 at 3:14 AM, Jeremy Kerr<jk(at)ozlabs(dot)org> wrote:
>> That code is not broken as it stands, and doesn't appear to really
>> gain anything from the proposed change.  Why should we risk any
>> portability questions when the code isn't going to get either simpler
>> or shorter?
>
> OK, after attempting a macro version of this, we end up with:
>
> #define DECLARE_SIGPIPE_INFO(var) struct sigpipe_info var;
>
> #define DISABLE_SIGPIPE(info, conn) \
>        ((SIGPIPE_MASKED(conn)) ? 0 : \
>                ((info)->got_epipe = false, \
>                pg_block_sigpipe(&(info)->oldsigmask, &(info)->sigpipe_pending)) < 0)
>
> - kinda ugly, uses the ?: and , operators to return the result of
> pg_block_sigpipe. We could avoid the comma with a block though.
>
> If we want to keep the current 'failaction' style of macro:
>
> #define DISABLE_SIGPIPE(info, conn, failaction) \
>   do { \
>                if (!SIGPIPE_MASKED(conn)) { \
>                        (info)->got_epipe = false; \
>                        if (pg_block_sigpipe(&(info)->oldsigmask, \
>                                        &(info)->sigpipe_pending)) < 0) { \
>                                failaction; \
>                        } \
>                } \
>        } while (0)
>
> We could ditch struct sigpipe info, but that means we need to declare
> three separate vars in DECLARE_SIGPIPE_INFO() instead of the one, and it
> doesn't clean up the macro code much. I'd rather reduce the amount of
> stuff that we declare "behind the caller's back".
>
> Compared to the static-function version:
>
> static inline int disable_sigpipe(PGconn *conn, struct sigpipe_info
> *info)
> {
>        if (sigpipe_masked(conn))
>                return 0;
>
>        info->got_epipe = false;
>        return pq_block_sigpipe(&info->oldsigmask, &info->sigpipe_pending) < 0;
> }
>
> Personally, I think the static functions are a lot cleaner, and don't
> think we lose any portability from using these (since inline is #define-
> ed out on compilers that don't support it). On non-inlining compilers,
> we do gain an extra function call, but we're about to enter the kernel
> anyway, so this will probably be lost in the noise (especially if we
> save the sigpipe-masking syscalls).
>
> But in the end, it's not up to me - do you still prefer the macro
> approach?

Since Tom seems to prefer the macro approach, and since the current
code uses a macro, I think we should stick with doing it that way.

Also, as some of Tom's comments above indicate, I don't think it's
making anything any easier for anyone that you keep submitting this as
two separate patches. It's one thing to submit a patch in pieces of
it is very large or complex and especially if the pieces are
independent, but that's not really the case here.

Because we are now over a week into this CommitFest, we need to get a
final, reviewable version of this patch as quickly as possible. So
please make the requested changes and resubmit as soon as you can.

Thanks,

...Robert

Currently, libpq will wrap each send() call on the connection with
two system calls to mask SIGPIPEs. This results in 3 syscalls instead
of one, and (on Linux) can lead to high contention on the signal
mask locks in threaded apps.

We have a couple of other methods to avoid SIGPIPEs:
sockopt(SO_NOSIGPIPE) and the MSG_NOSIGNAL flag to send().

This change attempts to use these if they're available at compile-
and run-time. If not, we drop back to manipulating the signal mask as
before.

Signed-off-by: Jeremy Kerr <jk(at)ozlabs(dot)org>

---
v4: roll into one patch, use macros

---
src/interfaces/libpq/fe-connect.c | 42 ++++++++++++
src/interfaces/libpq/fe-secure.c | 131 ++++++++++++++++++++++++++------------
src/interfaces/libpq/libpq-int.h | 2
3 files changed, 136 insertions(+), 39 deletions(-)

*** a/src/interfaces/libpq/fe-connect.c
--- b/src/interfaces/libpq/fe-connect.c
***************
*** 1089,1094 **** keep_going: /* We will come back to here until there is
--- 1089,1097 ----
while (conn->addr_cur != NULL)
{
struct addrinfo *addr_cur = conn->addr_cur;
+ #ifdef SO_NOSIGPIPE
+ int optval;
+ #endif /* SO_NOSIGPIPE */

/* Remember current address for possible error msg */
memcpy(&conn->raddr.addr, addr_cur->ai_addr,
***************
*** 1153,1158 **** keep_going: /* We will come back to here until there is
--- 1156,1200 ----
}
#endif /* F_SETFD */

+ /* We have three methods of blocking sigpipe during
+ * send() calls to this socket:
+ *
+ * - setsockopt(sock, SO_NOSIGPIPE)
+ * - send(sock, ..., MSG_NOSIGNAL)
+ * - setting the signal mask to SIG_IGN during send()
+ *
+ * The first two reduce the number of syscalls (for the
+ * third, we require three syscalls to implement a send()),
+ * so use them if they're available. Their availability is
+ * flagged in the following members of PGconn:
+ *
+ * conn->sigpipe_so - we have set up SO_NOSIGPIPE
+ * conn->sigpipe_flag - we're specifying MSG_NOSIGNAL
+ *
+ * If we can use SO_NOSIGPIPE, then set sigpipe_so here and
+ * we don't need to care about anything else. Otherwise,
+ * try MSG_NOSIGNAL by setting sigpipe_flag. If we get an
+ * error with MSG_NOSIGNAL, we clear the flag and revert
+ * to manual masking.
+ */
+ conn->sigpipe_so = false;
+ #ifdef MSG_NOSIGNAL
+ conn->sigpipe_flag = true;
+ #else /* !MSG_NOSIGNAL */
+ conn->sigpipe_flag = false;
+ #endif /* MSG_NOSIGNAL */
+
+ #ifdef SO_NOSIGPIPE
+ optval = 1;
+ if (!setsockopt(conn->sock, SOL_SOCKET, SO_NOSIGPIPE,
+ (char *)&optval, sizeof(optval)))
+ {
+ conn->sigpipe_so = true;
+ conn->sigpipe_flag = false;
+ }
+ #endif /* SO_NOSIGPIPE */
+
+
/*
* Start/make connection. This should not block, since we
* are in nonblock mode. If it does, well, too bad.
*** a/src/interfaces/libpq/fe-secure.c
--- b/src/interfaces/libpq/fe-secure.c
***************
*** 118,161 **** static long win32_ssl_create_mutex = 0;

/*
* Macros to handle disabling and then restoring the state of SIGPIPE handling.
- * Note that DISABLE_SIGPIPE() must appear at the start of a block.
*/

#ifndef WIN32
#ifdef ENABLE_THREAD_SAFETY

! #define DISABLE_SIGPIPE(failaction) \
! sigset_t osigmask; \
! bool sigpipe_pending; \
! bool got_epipe = false; \
! \
! if (pq_block_sigpipe(&osigmask, &sigpipe_pending) < 0) \
! failaction

! #define REMEMBER_EPIPE(cond) \
! do { \
! if (cond) \
! got_epipe = true; \
! } while (0)

! #define RESTORE_SIGPIPE() \
! pq_reset_sigpipe(&osigmask, sigpipe_pending, got_epipe)
! #else /* !ENABLE_THREAD_SAFETY */

! #define DISABLE_SIGPIPE(failaction) \
! pqsigfunc oldsighandler = pqsignal(SIGPIPE, SIG_IGN)

! #define REMEMBER_EPIPE(cond)

! #define RESTORE_SIGPIPE() \
! pqsignal(SIGPIPE, oldsighandler)
! #endif /* ENABLE_THREAD_SAFETY */
! #else /* WIN32 */

! #define DISABLE_SIGPIPE(failaction)
! #define REMEMBER_EPIPE(cond)
! #define RESTORE_SIGPIPE()
! #endif /* WIN32 */

/* ------------------------------------------------------------ */
/* Procedures common to all secure sessions */
--- 118,185 ----

/*
* Macros to handle disabling and then restoring the state of SIGPIPE handling.
*/

#ifndef WIN32
+
+ #ifdef USE_SSL
+ #define SIGPIPE_MASKED(c) \
+ ((c)->ssl ? (c)->sigpipe_so : (c)->sigpipe_so || (c)->sigpipe_flag)
+ #else
+ #define SIGPIPE_MASKED(c) \
+ ((c)->sigpipe_so || (c)->sigpipe_flag)
+ #endif
+
#ifdef ENABLE_THREAD_SAFETY

! struct sigpipe_info {
! sigset_t oldsigmask;
! bool sigpipe_pending;
! bool got_epipe;
! };

! #define DECLARE_SIGPIPE_INFO(var) struct sigpipe_info var

! #define DISABLE_SIGPIPE(conn, info) \
! ({ int __x = 0; \
! if (!SIGPIPE_MASKED(conn)) \
! (info).got_epipe = false; \
! __x = pq_block_sigpipe(&(info).oldsigmask, \
! &(info).sigpipe_pending) < 0; \
! __x; })

! #define REMEMBER_EPIPE(info, cond) \
! if (cond) (info).got_epipe = true

! #define RESTORE_SIGPIPE(conn, info) \
! if (!SIGPIPE_MASKED(conn)) \
! pq_reset_sigpipe(&(info).oldsigmask, (info).sigpipe_pending, \
! (info).got_epipe)

! #else /* !ENABLE_THREAD_SAFETY */
!
! #define DECLARE_SIGPIPE_INFO(var) pqsigfunc var = NULL
!
! #define DISABLE_SIGPIPE(conn, info) \
! ({ if (!SIGPIPE_MASKED(conn)) \
! (info) = pqsignal(SIGPIPE, SIG_IGN); \
! 0; })
!
! #define REMEMBER_EPIPE(info, cond)
!
! #define RESTORE_SIGPIPE(conn, info) \
! if (!SIGPIPE_MASKED(conn)) \
! pqsignal(SIGPIPE, (info))
!
! #endif /* ENABLE_THREAD_SAFETY */
! #else /* WIN32 */
!
! #define DECLARE_SIGPIPE_INFO(var)
! #define DISABLE_SIGPIPE(conn, info) 0
! #define REMEMBER_EPIPE(info, cond)
! #define RESTORE_SIGPIPE(conn, info)

! #endif /* WIN32 */

/* ------------------------------------------------------------ */
/* Procedures common to all secure sessions */
***************
*** 283,291 **** pqsecure_read(PGconn *conn, void *ptr, size_t len)
if (conn->ssl)
{
int err;

/* SSL_read can write to the socket, so we need to disable SIGPIPE */
! DISABLE_SIGPIPE(return -1);

rloop:
n = SSL_read(conn->ssl, ptr, len);
--- 307,317 ----
if (conn->ssl)
{
int err;
+ DECLARE_SIGPIPE_INFO(info);

/* SSL_read can write to the socket, so we need to disable SIGPIPE */
! if (DISABLE_SIGPIPE(conn, info))
! return -1;

rloop:
n = SSL_read(conn->ssl, ptr, len);
***************
*** 312,318 **** rloop:

if (n == -1)
{
! REMEMBER_EPIPE(SOCK_ERRNO == EPIPE);
printfPQExpBuffer(&conn->errorMessage,
libpq_gettext("SSL SYSCALL error: %s\n"),
SOCK_STRERROR(SOCK_ERRNO, sebuf, sizeof(sebuf)));
--- 338,344 ----

if (n == -1)
{
! REMEMBER_EPIPE(info, SOCK_ERRNO == EPIPE);
printfPQExpBuffer(&conn->errorMessage,
libpq_gettext("SSL SYSCALL error: %s\n"),
SOCK_STRERROR(SOCK_ERRNO, sebuf, sizeof(sebuf)));
***************
*** 348,354 **** rloop:
break;
}

! RESTORE_SIGPIPE();
}
else
#endif
--- 374,380 ----
break;
}

! RESTORE_SIGPIPE(conn, info);
}
else
#endif
***************
*** 364,377 **** ssize_t
pqsecure_write(PGconn *conn, const void *ptr, size_t len)
{
ssize_t n;
!
! DISABLE_SIGPIPE(return -1);

#ifdef USE_SSL
if (conn->ssl)
{
int err;

n = SSL_write(conn->ssl, ptr, len);
err = SSL_get_error(conn->ssl, n);
switch (err)
--- 390,405 ----
pqsecure_write(PGconn *conn, const void *ptr, size_t len)
{
ssize_t n;
! DECLARE_SIGPIPE_INFO(info);

#ifdef USE_SSL
if (conn->ssl)
{
int err;

+ if (DISABLE_SIGPIPE(conn, info))
+ return -1;
+
n = SSL_write(conn->ssl, ptr, len);
err = SSL_get_error(conn->ssl, n);
switch (err)
***************
*** 396,402 **** pqsecure_write(PGconn *conn, const void *ptr, size_t len)

if (n == -1)
{
! REMEMBER_EPIPE(SOCK_ERRNO == EPIPE);
printfPQExpBuffer(&conn->errorMessage,
libpq_gettext("SSL SYSCALL error: %s\n"),
SOCK_STRERROR(SOCK_ERRNO, sebuf, sizeof(sebuf)));
--- 424,430 ----

if (n == -1)
{
! REMEMBER_EPIPE(&info, SOCK_ERRNO == EPIPE);
printfPQExpBuffer(&conn->errorMessage,
libpq_gettext("SSL SYSCALL error: %s\n"),
SOCK_STRERROR(SOCK_ERRNO, sebuf, sizeof(sebuf)));
***************
*** 434,444 **** pqsecure_write(PGconn *conn, const void *ptr, size_t len)
else
#endif
{
! n = send(conn->sock, ptr, len, 0);
! REMEMBER_EPIPE(n < 0 && SOCK_ERRNO == EPIPE);
}

! RESTORE_SIGPIPE();

return n;
}
--- 462,496 ----
else
#endif
{
! int flags = 0;
!
! #ifdef MSG_NOSIGNAL
! if (!conn->sigpipe_so && conn->sigpipe_flag)
! flags |= MSG_NOSIGNAL;
! #endif /* MSG_NOSIGNAL */
!
! retry_masked:
! if (DISABLE_SIGPIPE(conn, info))
! return -1;
!
! n = send(conn->sock, ptr, len, flags);
!
! if (n < 0) {
! /* if we see an EINVAL, it may be because MSG_NOSIGNAL isn't
! * available on this machine. So, clear sigpipe_flag so we don't
! * try this flag again, and retry the send().
! */
! if (flags != 0 && SOCK_ERRNO == EINVAL) {
! conn->sigpipe_flag = false;
! flags = 0;
! goto retry_masked;
! }
!
! REMEMBER_EPIPE(info, SOCK_ERRNO == EPIPE);
! }
}

! RESTORE_SIGPIPE(conn, info);

return n;
}
***************
*** 1220,1233 **** close_SSL(PGconn *conn)
{
if (conn->ssl)
{
! DISABLE_SIGPIPE((void) 0);
SSL_shutdown(conn->ssl);
SSL_free(conn->ssl);
conn->ssl = NULL;
pqsecure_destroy();
/* We have to assume we got EPIPE */
! REMEMBER_EPIPE(true);
! RESTORE_SIGPIPE();
}

if (conn->peer)
--- 1272,1286 ----
{
if (conn->ssl)
{
! DECLARE_SIGPIPE_INFO(info);
! DISABLE_SIGPIPE(conn, info);
SSL_shutdown(conn->ssl);
SSL_free(conn->ssl);
conn->ssl = NULL;
pqsecure_destroy();
/* We have to assume we got EPIPE */
! REMEMBER_EPIPE(info, true);
! RESTORE_SIGPIPE(conn, info);
}

if (conn->peer)
*** a/src/interfaces/libpq/libpq-int.h
--- b/src/interfaces/libpq/libpq-int.h
***************
*** 341,346 **** struct pg_conn
--- 341,348 ----
ProtocolVersion pversion; /* FE/BE protocol version in use */
int sversion; /* server version, e.g. 70401 for 7.4.1 */
bool password_needed; /* true if server demanded a password */
+ bool sigpipe_so; /* have we masked sigpipes via SO_NOSIGPIPE? */
+ bool sigpipe_flag; /* can we mask sigpipes via MSG_NOSIGNAL? */

/* Transient state needed while establishing connection */
struct addrinfo *addrlist; /* list of possible backend addresses */

Jeremy Kerr <jk(at)ozlabs(dot)org> writes:
> Currently, libpq will wrap each send() call on the connection with
> two system calls to mask SIGPIPEs. This results in 3 syscalls instead
> of one, and (on Linux) can lead to high contention on the signal
> mask locks in threaded apps.

> We have a couple of other methods to avoid SIGPIPEs:
> sockopt(SO_NOSIGPIPE) and the MSG_NOSIGNAL flag to send().

> This change attempts to use these if they're available at compile-
> and run-time. If not, we drop back to manipulating the signal mask as
> before.

Applied with revisions --- those macro definitions were still a mess
:-(. In particular, ({...}) is a gcc-ism.

regards, tom lane