On Sat, Jan 02, 2021 at 12:07:28AM +0100, Theo Buehler wrote: > 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.
This is OK claudio@ > 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__); > > -- :wq Claudio
