From c596222847d17e37c213712299333338c76c9e2d Mon Sep 17 00:00:00 2001 From: Donald Buczek Date: Thu, 20 May 2021 16:14:32 +0200 Subject: [PATCH 1/7] mxshadowsrv: Use __atomic_fetch_sub instead of __sync_fetch_and_sub MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Quote gcc manual: "These functions are intended to replace the legacy ‘__sync’ builtins. The main difference is that the memory order that is requested is a parameter to the functions. New code should always use the ‘__atomic’ builtins rather than the ‘__sync’ builtins. Same assembler code generated on x86_64 for both builtins and no matter what memory order is specified: lock xaddl %eax, debug_remaining_connects(%rip) On ppc, however, __sync_fetch_and_sub creates code for sequential consistency and puts a "sync" instruction before the decrement (which is done atomicially between "lwarx" and "stwcx" instructions) and a "isync" instruction behind it. These two instructions are omitted when only relaxed consistency is requested: sync # optional L22: lwarx 9,0,27 addi 10,9,-1 stwcx. 10,0,27 bne 0,.L22 isync # optional Not, that it would matter at all :-) Make this change anyway to get used to the recommended builtins. --- mxshadowsrv.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/mxshadowsrv.c b/mxshadowsrv.c index 7e4614b..b8af131 100644 --- a/mxshadowsrv.c +++ b/mxshadowsrv.c @@ -193,7 +193,7 @@ static void *client_thread(void *arg) { while (1) { #ifdef DEBUG_MAX_CONNECTS - if ( __sync_fetch_and_sub(&debug_remaining_connects, 1) <= 0) + if ( __atomic_fetch_sub(&debug_remaining_connects, 1, __ATOMIC_RELAXED) <= 0) return NULL; #endif int _cleanup_(free_fd) socket = accept4(listen_socket, NULL, NULL, SOCK_NONBLOCK); From 34a9bf24ca85674918f85efc8dc3eb8edab42b1c Mon Sep 17 00:00:00 2001 From: Donald Buczek Date: Sat, 22 May 2021 11:42:53 +0200 Subject: [PATCH 2/7] mxshadowsrv: Do not log shadow reload to stderr --- mxshadowsrv.c | 1 - 1 file changed, 1 deletion(-) diff --git a/mxshadowsrv.c b/mxshadowsrv.c index b8af131..aef14a5 100644 --- a/mxshadowsrv.c +++ b/mxshadowsrv.c @@ -25,7 +25,6 @@ static char *map_shadow(char *filename, struct stat *statbufptr) { int fd; while (1) { while (1) { - fprintf(stderr, "loading %s\n", filename); fd = open(filename, O_RDONLY); if (fd != -1) break; From eef7f8f26728569026f2011cfc699c5487d7da98 Mon Sep 17 00:00:00 2001 From: Donald Buczek Date: Sun, 23 May 2021 12:18:32 +0200 Subject: [PATCH 3/7] common.h: Use better message for early client EOF When SSL sees a client hangup, SSL_get_error currently returns SSL_ERROR_SYSCALL. The page at [1] says in section BUGS, that errno would be 0 in this case: The SSL_ERROR_SYSCALL with errno value of 0 indicates unexpected EOF from the peer. To my experimentation, this is not true. errno is left unchanged instead. The page at [2] says, that ERR_get_error could be used to distinguish between EOF and some errno indicated failure: SSL_ERROR_SYSCALL Some I/O error occurred. The OpenSSL error queue may contain more information on the error. If the error queue is empty (i.e. ERR_get_error() returns 0), ret can be used to find out more about the error: If ret == 0, an EOF was observed that violates the protocol. If ret == -1, the underlying BIO reported an I/O error (for socket I/O on Unix systems, consult errno for details). But to my experimentation, this is not true either. In both cases, ERR_get_error doesn't have any further information and ret (from SSL_accept) is -1 in both cases. This might be fixed in OpenSLL 3.0 [3]: On an unexpected EOF, versions before OpenSSL 3.0 returned SSL_ERROR_SYSCALL, nothing was added to the error stack, and errno was 0. Since OpenSSL 3.0 the returned error is SSL_ERROR_SSL with a meaningful error on the error stack. Although it is ugly, we only have the option to set errno to zero before the operation. do this in ssl_read_with_timeout and ssl_accept_with_timeout. [1]: https://www.openssl.org/docs/man1.1.1/man3/SSL_get_error.html [2]: https://linux.die.net/man/3/ssl_get_error [3]: https://www.openssl.org/docs/manmaster/man3/SSL_get_error.html --- common.h | 9 ++++++++- 1 file changed, 8 insertions(+), 1 deletion(-) diff --git a/common.h b/common.h index 4bdb577..fd365e3 100644 --- a/common.h +++ b/common.h @@ -118,6 +118,7 @@ static int __attribute__((unused)) ssl_write_with_timeout(SSL *ssl, int fd, char } static int __attribute__((unused)) ssl_read_with_timeout(SSL *ssl, int fd, void *buf, size_t num, int timeout){ + errno = 0; /* see commit message */ while (1) { int status = SSL_read(ssl, buf, num); if (status > 0) @@ -130,7 +131,7 @@ static int __attribute__((unused)) ssl_read_with_timeout(SSL *ssl, int fd, void return -1; continue; case SSL_ERROR_SYSCALL: - if (errno==0) { + if (errno == 0) { COMMON_LOG(LOG_ERR, "%s: unexpected EOF from peer", __func__); errno = ECONNABORTED; return -1; @@ -148,6 +149,7 @@ static int __attribute__((unused)) ssl_read_with_timeout(SSL *ssl, int fd, void } static int __attribute__((unused)) ssl_accept_with_timeout(SSL *ssl, int fd, int timeout) { + errno = 0; /* see commit message */ while (1) { int status = SSL_accept(ssl); if (status == 1) @@ -160,6 +162,11 @@ static int __attribute__((unused)) ssl_accept_with_timeout(SSL *ssl, int fd, in return -1; continue; case SSL_ERROR_SYSCALL: + if (errno == 0) { + COMMON_LOG(LOG_ERR, "%s: unexpected EOF from peer", __func__); + errno = ECONNABORTED; + return -1; + } COMMON_LOG(LOG_ERR, "%s: %m", __func__); return -1; case SSL_ERROR_SSL: From 36d93d471476951bc530c6ea45df5a39ddb399e5 Mon Sep 17 00:00:00 2001 From: Donald Buczek Date: Sun, 23 May 2021 13:35:48 +0200 Subject: [PATCH 4/7] common.h: Log all failues of ssl_*_with_timeout Use COMMON_LOG to log failures from wait_rd_with_timeout which includes a timeout. --- common.h | 12 +++++++++--- 1 file changed, 9 insertions(+), 3 deletions(-) diff --git a/common.h b/common.h index fd365e3..380264c 100644 --- a/common.h +++ b/common.h @@ -99,8 +99,10 @@ static int __attribute__((unused)) ssl_write_with_timeout(SSL *ssl, int fd, char switch (ssl_error) { case SSL_ERROR_WANT_READ: status = wait_rd_with_timeout(fd, timeout); - if (status == -1) + if (status == -1) { + COMMON_LOG(LOG_ERR, "%s: %m", __func__); return -1; + } continue; case SSL_ERROR_SYSCALL: COMMON_LOG(LOG_ERR, "%s: %m", __func__); @@ -127,8 +129,10 @@ static int __attribute__((unused)) ssl_read_with_timeout(SSL *ssl, int fd, void switch (ssl_error) { case SSL_ERROR_WANT_READ: status = wait_rd_with_timeout(fd, timeout); - if (status == -1) + if (status == -1) { + COMMON_LOG(LOG_ERR, "%s: %m", __func__); return -1; + } continue; case SSL_ERROR_SYSCALL: if (errno == 0) { @@ -158,8 +162,10 @@ static int __attribute__((unused)) ssl_accept_with_timeout(SSL *ssl, int fd, in switch (ssl_error) { case SSL_ERROR_WANT_READ: status = wait_rd_with_timeout(fd, timeout); - if (status == -1) + if (status == -1) { + COMMON_LOG(LOG_ERR, "%s: %m", __func__); return -1; + } continue; case SSL_ERROR_SYSCALL: if (errno == 0) { From 19e11ec8c3bddbb327d0d067bbb81bdd4129c3cc Mon Sep 17 00:00:00 2001 From: Donald Buczek Date: Sun, 23 May 2021 13:41:14 +0200 Subject: [PATCH 5/7] common.h: Make error string optional The call `psslerror("")` prints a line with ":" only. Make the string optional so that NULL or "" won't output anything. --- common.h | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/common.h b/common.h index 380264c..fbe67af 100644 --- a/common.h +++ b/common.h @@ -44,7 +44,8 @@ static void __attribute__((unused)) free_string(char **ptr) { } static void __attribute__((unused)) psslerror(char *str) { - COMMON_LOG(LOG_ERR, "%s:", str); + if (str != NULL && strcmp(str, "") != 0) + COMMON_LOG(LOG_ERR, "%s:", str); unsigned long ssl_err; while ((ssl_err = ERR_get_error())) { COMMON_LOG(LOG_ERR, "ssl error: %lud:%s:%s:%s", From 8693da8beb31c4abf8b33515dcc8b97d528fe2ea Mon Sep 17 00:00:00 2001 From: Donald Buczek Date: Sun, 23 May 2021 13:45:23 +0200 Subject: [PATCH 6/7] mxshadowsrv: Avoid redundant error messages --- mxshadowsrv.c | 14 +++----------- 1 file changed, 3 insertions(+), 11 deletions(-) diff --git a/mxshadowsrv.c b/mxshadowsrv.c index aef14a5..f926a44 100644 --- a/mxshadowsrv.c +++ b/mxshadowsrv.c @@ -152,18 +152,13 @@ static void process_client(int socket) { if (ssl == NULL) { psslerror("SSL_new"); return; } SSL_set_fd(ssl, socket); if (ssl_accept_with_timeout(ssl, socket, TIMEOUT) <= 0) { - perror("accept"); return; } char buf[64]; int len = ssl_read_with_timeout(ssl, socket, buf, sizeof(buf), TIMEOUT); - if (len == 0) + if (len <= 0 ) return; - if (len < 0) { - perror("read"); - return; - } if (len == sizeof(buf)) { fprintf(stderr, "identifier to long\n"); SSL_shutdown(ssl); @@ -180,11 +175,8 @@ static void process_client(int socket) { status = pthread_mutex_unlock(&shadow_mutex); if (status != 0) { errno = status; perror("pthread_mutex_unlock"); exit(1);} - if (line_len) { - int status = ssl_write_with_timeout(ssl, socket, line, line_len, TIMEOUT); - if (status == -1) - perror("write"); - } + if (line_len) + ssl_write_with_timeout(ssl, socket, line, line_len, TIMEOUT); SSL_shutdown(ssl); } From 41fb1357cee00bccaa63cfe86e086fe63b7cc99c Mon Sep 17 00:00:00 2001 From: Donald Buczek Date: Tue, 25 May 2021 08:55:37 +0200 Subject: [PATCH 7/7] Makefile: Remove unneeded dependencies --- Makefile | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/Makefile b/Makefile index 08ca231..76528ff 100644 --- a/Makefile +++ b/Makefile @@ -38,8 +38,8 @@ libnss_mxshadow.so.2: libnss_mxshadow.c get_shadow_line.c common.h test_server: test_server.c get_shadow_line.c common.h gcc $(CFLAGS) -o test_server test_server.c -l:libssl.a -l:libcrypto.a -lpthread -ldl -test_query_shadow: test_query_shadow.c get_shadow_line.c common.h - gcc $(CFLAGS) -o test_query_shadow test_query_shadow.c -l:libssl.a -l:libcrypto.a -lpthread -ldl +test_query_shadow: test_query_shadow.c + gcc $(CFLAGS) -o test_query_shadow test_query_shadow.c mxshadowsrv: mxshadowsrv.c common.h gcc $(CFLAGS) -o mxshadowsrv mxshadowsrv.c -l:libssl.a -l:libcrypto.a -lpthread -ldl