On Fri, Jan 01, 2021 at 11:38:32PM +0100, Claudio Jeker wrote:
> 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).
Thanks, I was just wondering about this as well. The socket is of
course non-blocking and I used the blocking idiom. I do not how likely
it is that we hit an actual WANT_POLL* situation but it is conceivable.
In my few test cases, tls_close() always succeeds the first time around.
> 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.
tls_close() is known to need some work. So perhaps we can do this simple
diff in the interim. This addresses the annoying resource leaks I'm
seeing and the proper handling with libevent (which I'm unfamiliar with)
can be done once it's figured out.
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 22:55:29 -0000
@@ -1333,14 +1333,16 @@ 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)
+ tls_close(clt->clt_tls_ctx); /* XXX - error handling */
+ 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__);
