Re: [HACKERS] Simplify sleeping while reading/writing from client
On 02/06/2015 11:50 AM, Andres Freund wrote:
On 2015-02-05 16:45:50 +0200, Heikki Linnakangas wrote:
I propose the attached, which pulls all the wait-retry logic up to
secure_read() and secure_write(). This makes the code a lot more
understandable.
Generally a good idea. Especially if we get more ssl implementations.
But I think it'll make the already broken renegotiation handling even
worse. Because now we're always entering do_handshake in nonblocking
mode (essentially).
Good point. In the other thread, we concluded that the
SSL_do_handshake() call isn't really needed anyway, so attached are two
patches: 1. remove the SSL_do_handshake() calls, and 2. the previous
patch, to simplify the waiting logic.
- Heikki
From 30e5930c9da553d504339e00a05a2a8892182f63 Mon Sep 17 00:00:00 2001
From: Heikki Linnakangas heikki.linnakangas@iki.fi
Date: Thu, 12 Feb 2015 15:53:26 +0200
Subject: [PATCH 1/2] Simplify the way OpenSSL renegotiation is initiated in
server.
At least in all modern versions of OpenSSL, it is enough to call
SSL_renegotiate() once, and then forget about it. Subsequent SSL_write()
and SSL_read() calls will finish the handshake.
The SSL_set_session_id_context() call is unnecessary too. We only have
one SSL context, and the SSL session was created with that to begin with.
---
src/backend/libpq/be-secure-openssl.c | 23 ---
1 file changed, 23 deletions(-)
diff --git a/src/backend/libpq/be-secure-openssl.c b/src/backend/libpq/be-secure-openssl.c
index d5f9712..d13ce33 100644
--- a/src/backend/libpq/be-secure-openssl.c
+++ b/src/backend/libpq/be-secure-openssl.c
@@ -624,33 +624,10 @@ be_tls_write(Port *port, void *ptr, size_t len)
*/
SSL_clear_num_renegotiations(port-ssl);
- SSL_set_session_id_context(port-ssl, (void *) SSL_context,
- sizeof(SSL_context));
if (SSL_renegotiate(port-ssl) = 0)
ereport(COMMERROR,
(errcode(ERRCODE_PROTOCOL_VIOLATION),
errmsg(SSL failure during renegotiation start)));
- else
- {
- int retries;
-
- /*
- * A handshake can fail, so be prepared to retry it, but only
- * a few times.
- */
- for (retries = 0;; retries++)
- {
-if (SSL_do_handshake(port-ssl) 0)
- break; /* done */
-ereport(COMMERROR,
- (errcode(ERRCODE_PROTOCOL_VIOLATION),
- errmsg(SSL handshake failure on renegotiation, retrying)));
-if (retries = 20)
- ereport(FATAL,
- (errcode(ERRCODE_PROTOCOL_VIOLATION),
- errmsg(could not complete SSL handshake on renegotiation, too many failures)));
- }
- }
}
wloop:
--
2.1.4
From 6341bd76e8184c5c464c90a1ba3d99f02ef61bac Mon Sep 17 00:00:00 2001
From: Heikki Linnakangas heikki.linnakangas@iki.fi
Date: Thu, 5 Feb 2015 16:44:13 +0200
Subject: [PATCH 2/2] Simplify waiting logic in reading from / writing to
client.
The client socket is always in non-blocking mode, and if we actually want
blocking behaviour, we emulate it by sleeping and retrying. But we retry
loops at different layers for reads and writes, which was confusing.
To simplify, remove all the sleeping and retrying code from the lower
levels, from be_tls_read and secure_raw_read and secure_raw_write, and put
all the logic in secure_read() and secure_write().
---
src/backend/libpq/be-secure-openssl.c | 81 +--
src/backend/libpq/be-secure.c | 143 ++
src/backend/libpq/pqcomm.c| 3 +-
src/include/libpq/libpq-be.h | 4 +-
4 files changed, 79 insertions(+), 152 deletions(-)
diff --git a/src/backend/libpq/be-secure-openssl.c b/src/backend/libpq/be-secure-openssl.c
index d13ce33..37af6e4 100644
--- a/src/backend/libpq/be-secure-openssl.c
+++ b/src/backend/libpq/be-secure-openssl.c
@@ -511,14 +511,11 @@ be_tls_close(Port *port)
* Read data from a secure connection.
*/
ssize_t
-be_tls_read(Port *port, void *ptr, size_t len)
+be_tls_read(Port *port, void *ptr, size_t len, int *waitfor)
{
ssize_t n;
int err;
- int waitfor;
- int latchret;
-rloop:
errno = 0;
n = SSL_read(port-ssl, ptr, len);
err = SSL_get_error(port-ssl, n);
@@ -528,39 +525,15 @@ rloop:
port-count += n;
break;
case SSL_ERROR_WANT_READ:
+ *waitfor = WL_SOCKET_READABLE;
+ errno = EWOULDBLOCK;
+ n = -1;
+ break;
case SSL_ERROR_WANT_WRITE:
- /* Don't retry if the socket is in nonblocking mode. */
- if (port-noblock)
- {
-errno = EWOULDBLOCK;
-n = -1;
-break;
- }
-
- waitfor = WL_LATCH_SET;
-
- if (err == SSL_ERROR_WANT_READ)
-waitfor |= WL_SOCKET_READABLE;
- else
-waitfor |= WL_SOCKET_WRITEABLE;
-
- latchret = WaitLatchOrSocket(MyLatch, waitfor, port-sock, 0);
-
- /*
- * We'll, among other situations, get here if the low level
- * routine doing the actual recv() via the socket got interrupted
- * by a signal. That's so we can handle interrupts once outside
- * openssl, so we don't jump out from underneath its covers. We
- * can check this
Re: [HACKERS] Simplify sleeping while reading/writing from client
On 02/06/2015 10:38 AM, Michael Paquier wrote: On Thu, Feb 5, 2015 at 6:45 PM, Heikki Linnakangas wrote: Looking again at the code after Andres' interrupt-handling patch series, I got confused by the fact that there are several wait-retry loops in different layers, and reading and writing works slightly differently. I propose the attached, which pulls all the wait-retry logic up to secure_read() and secure_write(). This makes the code a lot more understandable. Are you sure that it is a good idea to move the check of port-noblock out of be_tls_[read|write] to an upper level? ISTM that we should set errno and n to respectively EWOULDBLOCK and -1 in be_tls_[write|read] when port-noblock and do nothing otherwise. In your patch those values are set even if the port is in block mode. It simplifies the code to do all the sleeping and interrupt handling code in the upper level, in secure_[read|write]. Do you see a problem with it? - Heikki -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Simplify sleeping while reading/writing from client
On Fri, Feb 6, 2015 at 1:46 PM, Heikki Linnakangas wrote: It simplifies the code to do all the sleeping and interrupt handling code in the upper level, in secure_[read|write]. Do you see a problem with it? Not directly. Reading the code I got uneasy with the fact that we fact unconditionally those parameters even if port is in block mode. -- Michael -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Simplify sleeping while reading/writing from client
On Thu, Feb 5, 2015 at 6:45 PM, Heikki Linnakangas wrote: Looking again at the code after Andres' interrupt-handling patch series, I got confused by the fact that there are several wait-retry loops in different layers, and reading and writing works slightly differently. I propose the attached, which pulls all the wait-retry logic up to secure_read() and secure_write(). This makes the code a lot more understandable. Are you sure that it is a good idea to move the check of port-noblock out of be_tls_[read|write] to an upper level? ISTM that we should set errno and n to respectively EWOULDBLOCK and -1 in be_tls_[write|read] when port-noblock and do nothing otherwise. In your patch those values are set even if the port is in block mode. -- Michael -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Simplify sleeping while reading/writing from client
On 2015-02-05 16:45:50 +0200, Heikki Linnakangas wrote: Looking again at the code after Andres' interrupt-handling patch series, I got confused by the fact that there are several wait-retry loops in different layers, and reading and writing works slightly differently. They don't really work all that differently? I propose the attached, which pulls all the wait-retry logic up to secure_read() and secure_write(). This makes the code a lot more understandable. Generally a good idea. Especially if we get more ssl implementations. But I think it'll make the already broken renegotiation handling even worse. Because now we're always entering do_handshake in nonblocking mode (essentially). Greetings, Andres Freund -- Andres Freund http://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Training Services -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
[HACKERS] Simplify sleeping while reading/writing from client
Looking again at the code after Andres' interrupt-handling patch series,
I got confused by the fact that there are several wait-retry loops in
different layers, and reading and writing works slightly differently.
I propose the attached, which pulls all the wait-retry logic up to
secure_read() and secure_write(). This makes the code a lot more
understandable.
- Heikki
From 4e73dbc72d279c2cb66af0a16357eab17f1c740b Mon Sep 17 00:00:00 2001
From: Heikki Linnakangas heikki.linnakangas@iki.fi
Date: Thu, 5 Feb 2015 16:44:13 +0200
Subject: [PATCH 1/1] Simplify waiting logic in reading from / writing to
client.
The client socket is always in non-blocking mode, and if we actually want
blocking behaviour, we emulate it by sleeping and retrying. But we retry
loops at different layers for reads and writes, which was confusing.
To simplify, remove all the sleeping and retrying code from the lower
levels, from be_tls_read and secure_raw_read and secure_raw_write, and put
all the logic in secure_read() and secure_write().
---
src/backend/libpq/be-secure-openssl.c | 81 +--
src/backend/libpq/be-secure.c | 143 ++
src/backend/libpq/pqcomm.c| 3 +-
src/include/libpq/libpq-be.h | 4 +-
4 files changed, 79 insertions(+), 152 deletions(-)
diff --git a/src/backend/libpq/be-secure-openssl.c b/src/backend/libpq/be-secure-openssl.c
index d5f9712..39587e8 100644
--- a/src/backend/libpq/be-secure-openssl.c
+++ b/src/backend/libpq/be-secure-openssl.c
@@ -511,14 +511,11 @@ be_tls_close(Port *port)
* Read data from a secure connection.
*/
ssize_t
-be_tls_read(Port *port, void *ptr, size_t len)
+be_tls_read(Port *port, void *ptr, size_t len, int *waitfor)
{
ssize_t n;
int err;
- int waitfor;
- int latchret;
-rloop:
errno = 0;
n = SSL_read(port-ssl, ptr, len);
err = SSL_get_error(port-ssl, n);
@@ -528,39 +525,15 @@ rloop:
port-count += n;
break;
case SSL_ERROR_WANT_READ:
+ *waitfor = WL_SOCKET_READABLE;
+ errno = EWOULDBLOCK;
+ n = -1;
+ break;
case SSL_ERROR_WANT_WRITE:
- /* Don't retry if the socket is in nonblocking mode. */
- if (port-noblock)
- {
-errno = EWOULDBLOCK;
-n = -1;
-break;
- }
-
- waitfor = WL_LATCH_SET;
-
- if (err == SSL_ERROR_WANT_READ)
-waitfor |= WL_SOCKET_READABLE;
- else
-waitfor |= WL_SOCKET_WRITEABLE;
-
- latchret = WaitLatchOrSocket(MyLatch, waitfor, port-sock, 0);
-
- /*
- * We'll, among other situations, get here if the low level
- * routine doing the actual recv() via the socket got interrupted
- * by a signal. That's so we can handle interrupts once outside
- * openssl, so we don't jump out from underneath its covers. We
- * can check this both, when reading and writing, because even
- * when writing that's just openssl's doing, not a 'proper' write
- * initiated by postgres.
- */
- if (latchret WL_LATCH_SET)
- {
-ResetLatch(MyLatch);
-ProcessClientReadInterrupt(true); /* preserves errno */
- }
- goto rloop;
+ *waitfor = WL_SOCKET_WRITEABLE;
+ errno = EWOULDBLOCK;
+ n = -1;
+ break;
case SSL_ERROR_SYSCALL:
/* leave it to caller to ereport the value of errno */
if (n != -1)
@@ -595,12 +568,10 @@ rloop:
* Write data to a secure connection.
*/
ssize_t
-be_tls_write(Port *port, void *ptr, size_t len)
+be_tls_write(Port *port, void *ptr, size_t len, int *waitfor)
{
ssize_t n;
int err;
- int waitfor;
- int latchret;
/*
* If SSL renegotiations are enabled and we're getting close to the
@@ -653,7 +624,6 @@ be_tls_write(Port *port, void *ptr, size_t len)
}
}
-wloop:
errno = 0;
n = SSL_write(port-ssl, ptr, len);
err = SSL_get_error(port-ssl, n);
@@ -663,30 +633,15 @@ wloop:
port-count += n;
break;
case SSL_ERROR_WANT_READ:
+ *waitfor = WL_SOCKET_READABLE;
+ errno = EWOULDBLOCK;
+ n = -1;
+ break;
case SSL_ERROR_WANT_WRITE:
-
- waitfor = WL_LATCH_SET;
-
- if (err == SSL_ERROR_WANT_READ)
-waitfor |= WL_SOCKET_READABLE;
- else
-waitfor |= WL_SOCKET_WRITEABLE;
-
- latchret = WaitLatchOrSocket(MyLatch, waitfor, port-sock, 0);
-
- /*
- * Check for interrupts here, in addition to secure_write(),
- * because an interrupted write in secure_raw_write() will return
- * here, and we cannot return to secure_write() until we've
- * written something.
- */
- if (latchret WL_LATCH_SET)
- {
-ResetLatch(MyLatch);
-ProcessClientWriteInterrupt(true); /* preserves errno */
- }
-
- goto wloop;
+ *waitfor = WL_SOCKET_WRITEABLE;
+ errno = EWOULDBLOCK;
+ n = -1;
+ break;
case SSL_ERROR_SYSCALL:
/* leave it to caller to ereport the value of errno */
if (n != -1)
diff --git a/src/backend/libpq/be-secure.c b/src/backend/libpq/be-secure.c
index c2c1842..4e7acbe 100644
--- a/src/backend/libpq/be-secure.c
+++ b/src/backend/libpq/be-secure.c
@@ -127,30 +127,45 @@ ssize_t
