On Fri, Jan 01, 2021 at 09:06:34PM +0100, Theo Buehler wrote:
> httpd(8) leaks resources when clients connect via TLS. The reason for
> this is that server_close() closes the socket underlying the TLS
> connection before calling tls_close().
>
> The currently unchecked tls_close() call fails with EBADF when trying to
> write out the close_notify in SSL_shutdown(). That resources are leaked
> are bugs in libssl/libtls which will need more investigation. Anyway,
> tls_close() wants an open socket if possible and it wants error
> checking, so move it up and do the usual dance.
>
> This diff makes a simple httpd run in essentially constant memory when I
> hammer it with many thousand TLS connections.
Is the fd passed to to tls_close() non-blocking? If so could result in a
busy loop on the while (ret == TLS_WANT_POLLIN || ret == TLS_WANT_POLLOUT).
Also doing these individual "poll" loops outside of the main event loop
can hurt performance since no other fd can be processed during that time.
I wondered about this in some other event based code I wrote. Especially
if tls_close() is called because of some error cleanup code.
> Index: server.c
> ===================================================================
> RCS file: /cvs/src/usr.sbin/httpd/server.c,v
> retrieving revision 1.122
> diff -u -p -r1.122 server.c
> --- server.c 31 Dec 2020 14:17:12 -0000 1.122
> +++ server.c 1 Jan 2021 18:03:32 -0000
> @@ -1333,14 +1333,25 @@ server_close(struct client *clt, const c
>
> if (clt->clt_srvbev != NULL)
> bufferevent_free(clt->clt_srvbev);
> +
> + /* tls_close must be called before the underlying socket is closed. */
> + if (clt->clt_tls_ctx != NULL) {
> + int ret;
> +
> + do {
> + ret = tls_close(clt->clt_tls_ctx);
> + } while (ret == TLS_WANT_POLLIN || ret == TLS_WANT_POLLOUT);
> + if (ret == -1) {
> + log_warnx("%s: tls_close failed (%s)", __func__,
> + tls_error(clt->clt_tls_ctx));
> + }
> + tls_free(clt->clt_tls_ctx);
> + }
> +
> if (clt->clt_fd != -1)
> close(clt->clt_fd);
> if (clt->clt_s != -1)
> close(clt->clt_s);
> -
> - if (clt->clt_tls_ctx != NULL)
> - tls_close(clt->clt_tls_ctx);
> - tls_free(clt->clt_tls_ctx);
>
> server_inflight_dec(clt, __func__);
>
>
--
:wq Claudio
