Location via proxy:   [ UP ]  
[Report a bug]   [Manage cookies]                
Skip to content

Commit 0a5c46a

Browse files
committed
Be more wary about OpenSSL not setting errno on error.
OpenSSL will sometimes return SSL_ERROR_SYSCALL without having set errno; this is apparently a reflection of recv(2)'s habit of not setting errno when reporting EOF. Ensure that we treat such cases the same as read EOF. Previously, we'd frequently report them like "could not accept SSL connection: Success" which is confusing, or worse report them with an unrelated errno left over from some previous syscall. To fix, ensure that errno is zeroed immediately before the call, and report its value only when it's not zero afterwards; otherwise report EOF. For consistency, I've applied the same coding pattern in libpq's pqsecure_raw_read(). Bare recv(2) shouldn't really return -1 without setting errno, but in case it does we might as well cope. Per report from Andres Freund. Back-patch to all supported versions. Discussion: https://postgr.es/m/20231208181451.deqnflwxqoehhxpe@awork3.anarazel.de
1 parent d3fe6e9 commit 0a5c46a

File tree

4 files changed

+45
-14
lines changed

4 files changed

+45
-14
lines changed

src/backend/libpq/be-secure-openssl.c

+11-4
Original file line numberDiff line numberDiff line change
@@ -460,6 +460,7 @@ be_tls_open_server(Port *port)
460460
* per-thread error queue following another call to an OpenSSL I/O
461461
* routine.
462462
*/
463+
errno = 0;
463464
ERR_clear_error();
464465
r = SSL_accept(port->ssl);
465466
if (r <= 0)
@@ -496,7 +497,7 @@ be_tls_open_server(Port *port)
496497
WAIT_EVENT_SSL_OPEN_SERVER);
497498
goto aloop;
498499
case SSL_ERROR_SYSCALL:
499-
if (r < 0)
500+
if (r < 0 && errno != 0)
500501
ereport(COMMERROR,
501502
(errcode_for_socket_access(),
502503
errmsg("could not accept SSL connection: %m")));
@@ -732,7 +733,7 @@ be_tls_read(Port *port, void *ptr, size_t len, int *waitfor)
732733
break;
733734
case SSL_ERROR_SYSCALL:
734735
/* leave it to caller to ereport the value of errno */
735-
if (n != -1)
736+
if (n != -1 || errno == 0)
736737
{
737738
errno = ECONNRESET;
738739
n = -1;
@@ -790,8 +791,14 @@ be_tls_write(Port *port, void *ptr, size_t len, int *waitfor)
790791
n = -1;
791792
break;
792793
case SSL_ERROR_SYSCALL:
793-
/* leave it to caller to ereport the value of errno */
794-
if (n != -1)
794+
795+
/*
796+
* Leave it to caller to ereport the value of errno. However, if
797+
* errno is still zero then assume it's a read EOF situation, and
798+
* report ECONNRESET. (This seems possible because SSL_write can
799+
* also do reads.)
800+
*/
801+
if (n != -1 || errno == 0)
795802
{
796803
errno = ECONNRESET;
797804
n = -1;

src/backend/libpq/pqcomm.c

+16-6
Original file line numberDiff line numberDiff line change
@@ -936,6 +936,8 @@ pq_recvbuf(void)
936936
{
937937
int r;
938938

939+
errno = 0;
940+
939941
r = secure_read(MyProcPort, PqRecvBuffer + PqRecvLength,
940942
PQ_RECV_BUFFER_SIZE - PqRecvLength);
941943

@@ -948,10 +950,13 @@ pq_recvbuf(void)
948950
* Careful: an ereport() that tries to write to the client would
949951
* cause recursion to here, leading to stack overflow and core
950952
* dump! This message must go *only* to the postmaster log.
953+
*
954+
* If errno is zero, assume it's EOF and let the caller complain.
951955
*/
952-
ereport(COMMERROR,
953-
(errcode_for_socket_access(),
954-
errmsg("could not receive data from client: %m")));
956+
if (errno != 0)
957+
ereport(COMMERROR,
958+
(errcode_for_socket_access(),
959+
errmsg("could not receive data from client: %m")));
955960
return EOF;
956961
}
957962
if (r == 0)
@@ -1028,6 +1033,8 @@ pq_getbyte_if_available(unsigned char *c)
10281033
/* Put the socket into non-blocking mode */
10291034
socket_set_nonblocking(true);
10301035

1036+
errno = 0;
1037+
10311038
r = secure_read(MyProcPort, c, 1);
10321039
if (r < 0)
10331040
{
@@ -1044,10 +1051,13 @@ pq_getbyte_if_available(unsigned char *c)
10441051
* Careful: an ereport() that tries to write to the client would
10451052
* cause recursion to here, leading to stack overflow and core
10461053
* dump! This message must go *only* to the postmaster log.
1054+
*
1055+
* If errno is zero, assume it's EOF and let the caller complain.
10471056
*/
1048-
ereport(COMMERROR,
1049-
(errcode_for_socket_access(),
1050-
errmsg("could not receive data from client: %m")));
1057+
if (errno != 0)
1058+
ereport(COMMERROR,
1059+
(errcode_for_socket_access(),
1060+
errmsg("could not receive data from client: %m")));
10511061
r = EOF;
10521062
}
10531063
}

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

+11-4
Original file line numberDiff line numberDiff line change
@@ -200,7 +200,7 @@ pgtls_read(PGconn *conn, void *ptr, size_t len)
200200
*/
201201
goto rloop;
202202
case SSL_ERROR_SYSCALL:
203-
if (n < 0)
203+
if (n < 0 && SOCK_ERRNO != 0)
204204
{
205205
result_errno = SOCK_ERRNO;
206206
if (result_errno == EPIPE ||
@@ -301,7 +301,13 @@ pgtls_write(PGconn *conn, const void *ptr, size_t len)
301301
n = 0;
302302
break;
303303
case SSL_ERROR_SYSCALL:
304-
if (n < 0)
304+
305+
/*
306+
* If errno is still zero then assume it's a read EOF situation,
307+
* and report EOF. (This seems possible because SSL_write can
308+
* also do reads.)
309+
*/
310+
if (n < 0 && SOCK_ERRNO != 0)
305311
{
306312
result_errno = SOCK_ERRNO;
307313
if (result_errno == EPIPE || result_errno == ECONNRESET)
@@ -1510,11 +1516,12 @@ open_client_SSL(PGconn *conn)
15101516
* was using the system CA pool. For other errors, log
15111517
* them using the normal SYSCALL logging.
15121518
*/
1513-
if (!save_errno && vcode == X509_V_ERR_UNABLE_TO_GET_ISSUER_CERT_LOCALLY &&
1519+
if (save_errno == 0 &&
1520+
vcode == X509_V_ERR_UNABLE_TO_GET_ISSUER_CERT_LOCALLY &&
15141521
strcmp(conn->sslrootcert, "system") == 0)
15151522
libpq_append_conn_error(conn, "SSL error: certificate verify failed: %s",
15161523
X509_verify_cert_error_string(vcode));
1517-
else if (r == -1)
1524+
else if (r == -1 && save_errno != 0)
15181525
libpq_append_conn_error(conn, "SSL SYSCALL error: %s",
15191526
SOCK_STRERROR(save_errno, sebuf, sizeof(sebuf)));
15201527
else

src/interfaces/libpq/fe-secure.c

+7
Original file line numberDiff line numberDiff line change
@@ -211,6 +211,8 @@ pqsecure_raw_read(PGconn *conn, void *ptr, size_t len)
211211
int result_errno = 0;
212212
char sebuf[PG_STRERROR_R_BUFLEN];
213213

214+
SOCK_ERRNO_SET(0);
215+
214216
n = recv(conn->sock, ptr, len, 0);
215217

216218
if (n < 0)
@@ -237,6 +239,11 @@ pqsecure_raw_read(PGconn *conn, void *ptr, size_t len)
237239
"\tbefore or while processing the request.");
238240
break;
239241

242+
case 0:
243+
/* If errno didn't get set, treat it as regular EOF */
244+
n = 0;
245+
break;
246+
240247
default:
241248
libpq_append_conn_error(conn, "could not receive data from server: %s",
242249
SOCK_STRERROR(result_errno,

0 commit comments

Comments
 (0)