On Wed, Apr 21, 2021 at 11:21:51AM +0200, Eric Faurot wrote:
> There is actually no reason to defer calls to tls_accept_socket() and
> tls_connect_socket() in an event callback. The code can be simplified
> by a great deal. It also eliminates the issue of keeping a reference
> to the listener tls context in the io structure.
Did anyone had a chance to look at it?
> Eric.
>
>
> Index: ioev.c
> ===================================================================
> RCS file: /cvs/src/usr.sbin/smtpd/ioev.c,v
> retrieving revision 1.45
> diff -u -p -r1.45 ioev.c
> --- ioev.c 5 Apr 2021 15:50:11 -0000 1.45
> +++ ioev.c 21 Apr 2021 08:35:29 -0000
> @@ -64,7 +64,6 @@ struct io {
> int state;
> struct event ev;
> struct tls *tls;
> - char *name;
>
> const char *error; /* only valid immediately on callback */
> };
> @@ -280,7 +279,6 @@ io_free(struct io *io)
> io->sock = -1;
> }
>
> - free(io->name);
> iobuf_clear(&io->iobuf);
> free(io);
> }
> @@ -817,14 +815,14 @@ io_connect_tls(struct io *io, struct tls
> if (io->tls)
> errx(1, "io_connect_tls: TLS already started");
>
> - if (hostname) {
> - if ((io->name = strdup(hostname)) == NULL)
> - err(1, "io_connect_tls");
> + if (tls_connect_socket(tls, io->sock, hostname) == -1) {
> + io->error = tls_error(tls);
> + return (-1);
> }
>
> io->tls = tls;
> io->state = IO_STATE_CONNECT_TLS;
> - io_reset(io, EV_WRITE, io_dispatch_connect_tls);
> + io_reset(io, EV_READ|EV_WRITE, io_dispatch_handshake_tls);
>
> return (0);
> }
> @@ -840,9 +838,14 @@ io_accept_tls(struct io *io, struct tls
>
> if (io->tls)
> errx(1, "io_accept_tls: TLS already started");
> - io->tls = tls;
> +
> + if (tls_accept_socket(tls, &io->tls, io->sock) == -1) {
> + io->error = tls_error(tls);
> + return (-1);
> + }
> +
> io->state = IO_STATE_ACCEPT_TLS;
> - io_reset(io, EV_READ, io_dispatch_accept_tls);
> + io_reset(io, EV_READ|EV_WRITE, io_dispatch_handshake_tls);
>
> return (0);
> }
> @@ -880,60 +883,6 @@ io_dispatch_handshake_tls(int fd, short
> }
>
> void
> -io_dispatch_accept_tls(int fd, short event, void *humppa)
> -{
> - struct io *io = humppa;
> - struct tls *tls = io->tls;
> - int ret;
> -
> - io_frame_enter("io_dispatch_accept_tls", io, event);
> -
> - /* Replaced by TLS context for accepted socket on success. */
> - io->tls = NULL;
> -
> - if (event == EV_TIMEOUT) {
> - io_callback(io, IO_TIMEOUT);
> - goto leave;
> - }
> -
> - if ((ret = tls_accept_socket(tls, &io->tls, io->sock)) == 0) {
> - io_reset(io, EV_READ|EV_WRITE, io_dispatch_handshake_tls);
> - goto leave;
> - }
> - io->error = tls_error(tls);
> - io_callback(io, IO_ERROR);
> -
> - leave:
> - io_frame_leave(io);
> - return;
> -}
> -
> -void
> -io_dispatch_connect_tls(int fd, short event, void *humppa)
> -{
> - struct io *io = humppa;
> - int ret;
> -
> - io_frame_enter("io_dispatch_connect_tls", io, event);
> -
> - if (event == EV_TIMEOUT) {
> - io_callback(io, IO_TIMEOUT);
> - goto leave;
> - }
> -
> - if ((ret = tls_connect_socket(io->tls, io->sock, io->name)) == 0) {
> - io_reset(io, EV_READ|EV_WRITE, io_dispatch_handshake_tls);
> - goto leave;
> - }
> -
> - io->error = tls_error(io->tls);
> - io_callback(io, IO_ERROR);
> -
> - leave:
> - io_frame_leave(io);
> -}
> -
> -void
> io_dispatch_read_tls(int fd, short event, void *humppa)
> {
> struct io *io = humppa;
> @@ -1017,37 +966,20 @@ io_dispatch_write_tls(int fd, short even
> void
> io_reload_tls(struct io *io)
> {
> - short ev = 0;
> - void (*dispatch)(int, short, void*) = NULL;
> -
> - switch (io->state) {
> - case IO_STATE_CONNECT_TLS:
> - ev = EV_WRITE;
> - dispatch = io_dispatch_connect_tls;
> - break;
> - case IO_STATE_ACCEPT_TLS:
> - ev = EV_READ;
> - dispatch = io_dispatch_accept_tls;
> - break;
> - case IO_STATE_UP:
> - ev = 0;
> - if (IO_READING(io) && !(io->flags & IO_PAUSE_IN)) {
> - ev = EV_READ;
> - dispatch = io_dispatch_read_tls;
> - }
> - else if (IO_WRITING(io) && !(io->flags & IO_PAUSE_OUT) &&
> - io_queued(io)) {
> - ev = EV_WRITE;
> - dispatch = io_dispatch_write_tls;
> - }
> - if (!ev)
> - return; /* paused */
> - break;
> - default:
> + if (io->state != IO_STATE_UP)
> errx(1, "io_reload_tls: bad state");
> +
> + if (IO_READING(io) && !(io->flags & IO_PAUSE_IN)) {
> + io_reset(io, EV_READ, io_dispatch_read_tls);
> + return;
> + }
> +
> + if (IO_WRITING(io) && !(io->flags & IO_PAUSE_OUT) && io_queued(io)) {
> + io_reset(io, EV_WRITE, io_dispatch_write_tls);
> + return;
> }
>
> - io_reset(io, ev, dispatch);
> + /* paused */
> }
>
> #endif /* IO_TLS */
> Index: mta_session.c
> ===================================================================
> RCS file: /cvs/src/usr.sbin/smtpd/mta_session.c,v
> retrieving revision 1.140
> diff -u -p -r1.140 mta_session.c
> --- mta_session.c 7 Mar 2021 20:56:41 -0000 1.140
> +++ mta_session.c 21 Apr 2021 08:18:56 -0000
> @@ -1596,7 +1596,11 @@ mta_tls_init(struct mta_session *s)
> return;
> }
>
> - io_connect_tls(s->io, tls, s->mxname);
> + if (io_connect_tls(s->io, tls, s->mxname) == -1) {
> + log_info("%016"PRIx64" mta closing reason=tls-connect-failed",
> s->id);
> + tls_free(tls);
> + mta_free(s);
> + }
> }
>
> static void
> Index: smtp_session.c
> ===================================================================
> RCS file: /cvs/src/usr.sbin/smtpd/smtp_session.c,v
> retrieving revision 1.429
> diff -u -p -r1.429 smtp_session.c
> --- smtp_session.c 5 Mar 2021 12:37:32 -0000 1.429
> +++ smtp_session.c 21 Apr 2021 08:20:44 -0000
> @@ -1067,7 +1067,12 @@ static void
> smtp_tls_init(struct smtp_session *s)
> {
> io_set_read(s->io);
> - io_accept_tls(s->io, s->listener->tls);
> + if (io_accept_tls(s->io, s->listener->tls) == -1) {
> + log_info("%016"PRIx64" smtp disconnected "
> + "reason=tls-accept-failed",
> + s->id);
> + smtp_free(s, "accept failed");
> + }
> }
>
> static void
>
>
