Re: httpd: drain abort response via bufferevent

On Thu, 25 Jun 2026 13:03:33 +0200,
Rafael Sadowski <[email protected]> wrote:
> 
> This is an all-in-one diff that replaces server_dump() with a
> bufferevent aka event-loop approach.
> 
> server_dump() did one non-blocking write(2)/tls_write(3) and discarded
> the return value, so partial writes were lost, meaning that large
> user-defined error documents were silently truncated in transit before.
> 
> This diff merge three commits into one. I don’t think it's too long to
> review. If we're OK, I'll split it into three separate commits:
>

Make sense for me.

I see logic in old behaviour, but this one is probably better.

So, OK kirill@

> 1. ref: server_create_builtin
> https://rsadowski.gothub.org/?action=diff&commit=7dc036be4782f5e6a31fa6bdd9ba1a57bea72f5a&path=httpd.git
> 2. drain abort response via bufferevent
> https://rsadowski.gothub.org/?action=diff&commit=b07039fc1f28bfe260ab72963e644c7575cad267&path=httpd.git
> 3. error loading the document in server_create_errdoc()
> https://rsadowski.gothub.org/?action=diff&commit=ea58fa5107a8182e42d5c180ce80c23fed0ad7d2&path=httpd.git
> 
> Cheers, Rafael
> 
> diff --git a/httpd.h b/httpd.h
> index 12122c6..664524e 100644
> --- a/httpd.h
> +++ b/httpd.h
> @@ -344,6 +344,8 @@ struct client {
>  	unsigned int		 clt_pipelining;
>  	int			 clt_line;
>  	int			 clt_done;
> +	int			 clt_close_after_write;
> +	char			*clt_close_msg;
>  	int			 clt_chunk;
>  	int			 clt_inflight;
>  	int			 clt_fcgi_count;
> @@ -651,7 +653,6 @@ void	 server_log(struct client *, const char *);
>  void	 server_sendlog(struct server_config *, int, const char *, ...)
>  	    __attribute__((__format__ (printf, 3, 4)));
>  void	 server_close(struct client *, const char *);
> -void	 server_dump(struct client *, const void *, size_t);
>  int	 server_client_cmp(struct client *, struct client *);
>  int	 server_bufferevent_printf(struct client *, const char *, ...)
>  	    __attribute__((__format__ (printf, 2, 3)));
> @@ -662,6 +663,7 @@ int	 server_bufferevent_write_chunk(struct client *,
>  	    struct evbuffer *, size_t);
>  int	 server_bufferevent_add(struct event *, int);
>  int	 server_bufferevent_write(struct client *, void *, size_t);
> +int	 server_bufferevent_write_close(struct client *, void *, size_t);
>  struct server *
>  	 server_byaddr(struct sockaddr *, in_port_t);
>  struct server_config *
> diff --git a/server.c b/server.c
> index 0f42ccd..43ea95e 100644
> --- a/server.c
> +++ b/server.c
> @@ -937,6 +937,12 @@ server_write(struct bufferevent *bev, void *arg)
>  	struct client		*clt = arg;
>  	struct evbuffer		*dst = EVBUFFER_OUTPUT(bev);
>  
> +	if (EVBUFFER_LENGTH(dst) == 0 && clt->clt_close_after_write) {
> +		server_close(clt, clt->clt_close_msg != NULL ?
> +		    clt->clt_close_msg : "response sent");
> +		return;
> +	}
> +
>  	if (EVBUFFER_LENGTH(dst) == 0 &&
>  	    clt->clt_toread == TOREAD_HTTP_NONE)
>  		goto done;
> @@ -957,24 +963,6 @@ server_write(struct bufferevent *bev, void *arg)
>  	return;
>  }
>  
> -void
> -server_dump(struct client *clt, const void *buf, size_t len)
> -{
> -	if (!len)
> -		return;
> -
> -	/*
> -	 * This function will dump the specified message directly
> -	 * to the underlying client, without waiting for success
> -	 * of non-blocking events etc. This is useful to print an
> -	 * error message before gracefully closing the client.
> -	 */
> -	if (clt->clt_tls_ctx != NULL)
> -		(void)tls_write(clt->clt_tls_ctx, buf, len);
> -	else
> -		(void)write(clt->clt_s, buf, len);
> -}
> -
>  void
>  server_read(struct bufferevent *bev, void *arg)
>  {
> @@ -1340,6 +1328,8 @@ server_close(struct client *clt, const char *msg)
>  	if (clt->clt_log != NULL)
>  		evbuffer_free(clt->clt_log);
>  
> +	free(clt->clt_close_msg);
> +
>  	free(clt);
>  	server_clients--;
>  }
> @@ -1472,6 +1462,20 @@ server_bufferevent_write(struct client *clt, void *data, size_t size)
>  	return (bufferevent_write(clt->clt_bev, data, size));
>  }
>  
> +int
> +server_bufferevent_write_close(struct client *clt, void *data, size_t size)
> +{
> +	if (clt->clt_bev == NULL)
> +		return (-1);
> +	if (clt->clt_close_after_write)
> +		return (-1);
> +
> +	clt->clt_persist = 0;
> +	clt->clt_close_after_write = 1;
> +
> +	return (bufferevent_write(clt->clt_bev, data, size));
> +}
> +
>  int
>  server_client_cmp(struct client *a, struct client *b)
>  {
> diff --git a/server_http.c b/server_http.c
> index 5ecb65c..28667bf 100644
> --- a/server_http.c
> +++ b/server_http.c
> @@ -55,6 +55,10 @@ char		*server_expand_http(struct client *, const char *,
>  		    char *, size_t);
>  char		*replace_var(char *, const char *, const char *);
>  char		*read_errdoc(const char *, const char *);
> +ssize_t		 server_create_builtin(struct server_config *, char **,
> +		    unsigned int, const char *);
> +char		*server_create_errdoc(struct server_config *, unsigned int,
> +		    const char *);
>  
>  static struct http_method	 http_methods[] = HTTP_METHODS;
>  static struct http_error	 http_errors[] = HTTP_ERRORS;
> @@ -880,15 +884,15 @@ server_abort_http(struct client *clt, unsigned int code, const char *msg)
>  	struct server_config	*srv_conf = clt->clt_srv_conf;
>  	struct bufferevent	*bev = clt->clt_bev;
>  	struct http_descriptor	*desc = clt->clt_descreq;
> -	const char		*httperr = NULL, *style;
> +	const char		*httperr = NULL;
>  	char			*httpmsg, *body = NULL, *extraheader = NULL;
>  	char			 tmbuf[32], hbuf[128], *hstsheader = NULL;
>  	char			*clenheader = NULL;
> -	char			*bannerheader = NULL, *bannertoken = NULL;
> +	char			*bannerheader = NULL;
>  	char			 buf[IBUF_READ_SIZE];
>  	char			*escapedmsg = NULL;
> -	char			 cstr[5];
>  	ssize_t			 bodylen;
> +	ssize_t			 httpmsglen;
>  
>  	if (code == 0) {
>  		server_close(clt, "dropped");
> @@ -941,6 +945,7 @@ server_abort_http(struct client *clt, unsigned int code, const char *msg)
>  			code = 500;
>  			extraheader = NULL;
>  		}
> +		free(escapedmsg);
>  		break;
>  	case 416:
>  		if (msg == NULL)
> @@ -962,66 +967,12 @@ server_abort_http(struct client *clt, unsigned int code, const char *msg)
>  		break;
>  	}
>  
> -	free(escapedmsg);
> -
> -	if ((srv_conf->flags & SRVFLAG_ERRDOCS) == 0)
> -		goto builtin; /* errdocs not enabled */
> -	if ((size_t)snprintf(cstr, sizeof(cstr), "%03u", code) >= sizeof(cstr))
> -		goto builtin;
> -
> -	if ((body = read_errdoc(srv_conf->errdocroot, cstr)) == NULL &&
> -	    (body = read_errdoc(srv_conf->errdocroot, HTTPD_ERRDOCTEMPLATE))
> -	    == NULL)
> -		goto builtin;
> -
> -	body = replace_var(body, "$HTTP_ERROR", httperr);
> -	body = replace_var(body, "$RESPONSE_CODE", cstr);
> -	/* Check if server banner is suppressed */
> -	if ((srv_conf->flags & SRVFLAG_NO_BANNER) == 0)
> -		body = replace_var(body, "$SERVER_SOFTWARE", HTTPD_SERVERNAME);
> -	else
> -		body = replace_var(body, "$SERVER_SOFTWARE", "");
> -	bodylen = strlen(body);
> -	goto send;
> -
> - builtin:
> -	/* A CSS stylesheet allows minimal customization by the user */
> -	style = "body { background-color: white; color: black; font-family: "
> -	    "'Comic Sans MS', 'Chalkboard SE', 'Comic Neue', sans-serif; }\n"
> -	    "hr { border: 0; border-bottom: 1px dashed; }\n"
> -	    "@media (prefers-color-scheme: dark) {\n"
> -	    "body { background-color: #1E1F21; color: #EEEFF1; }\n"
> -	    "a { color: #BAD7FF; }\n}";
> -
> -	/* If banner is suppressed, don't write it to the error document */
> -	if ((srv_conf->flags & SRVFLAG_NO_BANNER) == 0)
> -		if (asprintf(&bannertoken, "<hr>\n<address>%s</address>\n",
> -		    HTTPD_SERVERNAME) == -1) {
> -			bannertoken = NULL;
> -			goto done;
> -		}
> -
> -	/* Generate simple HTML error document */
> -	if ((bodylen = asprintf(&body,
> -	    "<!DOCTYPE html>\n"
> -	    "<html>\n"
> -	    "<head>\n"
> -	    "<meta charset=\"utf-8\">\n"
> -	    "<title>%03d %s</title>\n"
> -	    "<style type=\"text/css\"><!--\n%s\n--></style>\n"
> -	    "</head>\n"
> -	    "<body>\n"
> -	    "<h1>%03d %s</h1>\n"
> -	    "%s"
> -	    "</body>\n"
> -	    "</html>\n",
> -	    code, httperr, style, code, httperr,
> -	    bannertoken == NULL ? "" : bannertoken)) == -1) {
> -		body = NULL;
> +	if ((body = server_create_errdoc(srv_conf, code, httperr)) != NULL) {
> +		bodylen = strlen(body);
> +	} else if ((bodylen = server_create_builtin(srv_conf, &body, code,
> +	    httperr)) == -1)
>  		goto done;
> -	}
>  
> - send:
>  	if (srv_conf->flags & SRVFLAG_SERVER_HSTS &&
>  	    srv_conf->flags & SRVFLAG_TLS) {
>  		if (asprintf(&hstsheader, "Strict-Transport-Security: "
> @@ -1054,7 +1005,7 @@ server_abort_http(struct client *clt, unsigned int code, const char *msg)
>  		}
>  
>  	/* Add basic HTTP headers */
> -	if (asprintf(&httpmsg,
> +	if ((httpmsglen = asprintf(&httpmsg,
>  	    "HTTP/1.0 %03d %s\r\n"
>  	    "Date: %s\r\n"
>  	    "%s"
> @@ -1071,12 +1022,27 @@ server_abort_http(struct client *clt, unsigned int code, const char *msg)
>  	    extraheader == NULL ? "" : extraheader,
>  	    hstsheader == NULL ? "" : hstsheader,
>  	    desc->http_method == HTTP_METHOD_HEAD || clenheader == NULL ?
> -	    "" : body) == -1)
> +	    "" : body)) == -1)
>  		goto done;
>  
> -	/* Dump the message without checking for success */
> -	server_dump(clt, httpmsg, strlen(httpmsg));
> +	free(clt->clt_close_msg);
> +	if (asprintf(&clt->clt_close_msg, "%s (%03d %s)",
> +	    msg == NULL ? "\"\"" : msg, code, httperr) == -1)
> +		clt->clt_close_msg = NULL;
> +
> +	if (server_bufferevent_write_close(clt, httpmsg,
> +	    (size_t)httpmsglen) == -1) {
> +		/* fall back to synchronous close */
> +		free(httpmsg);
> +		goto done;
> +	}
>  	free(httpmsg);
> +	free(body);
> +	free(extraheader);
> +	free(hstsheader);
> +	free(clenheader);
> +	free(bannerheader);
> +	return;
>  
>   done:
>  	free(body);
> @@ -1084,7 +1050,6 @@ server_abort_http(struct client *clt, unsigned int code, const char *msg)
>  	free(hstsheader);
>  	free(clenheader);
>  	free(bannerheader);
> -	free(bannertoken);
>  	if (msg == NULL)
>  		msg = "\"\"";
>  	if (asprintf(&httpmsg, "%s (%03d %s)", msg, code, httperr) == -1) {
> @@ -1095,6 +1060,80 @@ server_abort_http(struct client *clt, unsigned int code, const char *msg)
>  	}
>  }
>  
> +char *
> +server_create_errdoc(struct server_config *srv_conf, unsigned int code,
> +    const char *httperr)
> +{
> +	char	 cstr[5];
> +	char	*body;
> +
> +	if (!(srv_conf->flags & SRVFLAG_ERRDOCS))
> +		return (NULL);
> +
> +	if ((size_t)snprintf(cstr, sizeof(cstr), "%03u", code) >= sizeof(cstr))
> +		return (NULL);
> +
> +	if ((body = read_errdoc(srv_conf->errdocroot, cstr)) == NULL &&
> +	    (body = read_errdoc(srv_conf->errdocroot, HTTPD_ERRDOCTEMPLATE))
> +	    == NULL)
> +		return (NULL);
> +
> +	body = replace_var(body, "$HTTP_ERROR", httperr);
> +	body = replace_var(body, "$RESPONSE_CODE", cstr);
> +	body = replace_var(body, "$SERVER_SOFTWARE",
> +	    (srv_conf->flags & SRVFLAG_NO_BANNER) ? "" : HTTPD_SERVERNAME);
> +
> +	return (body);
> +}
> +
> +ssize_t
> +server_create_builtin(struct server_config *srv_conf, char **body,
> +    unsigned int code, const char *httperr)
> +{
> +	const char	*style;
> +	char		*bannertoken = NULL;
> +	ssize_t		 bodylen;
> +
> +	/* A CSS stylesheet allows minimal customization by the user */
> +	style = "body { background-color: white; color: black; font-family: "
> +	    "'Comic Sans MS', 'Chalkboard SE', 'Comic Neue', sans-serif; }\n"
> +	    "hr { border: 0; border-bottom: 1px dashed; }\n"
> +	    "@media (prefers-color-scheme: dark) {\n"
> +	    "body { background-color: #1E1F21; color: #EEEFF1; }\n"
> +	    "a { color: #BAD7FF; }\n}";
> +
> +	/* If banner is suppressed, don't write it to the error document */
> +	if ((srv_conf->flags & SRVFLAG_NO_BANNER) == 0) {
> +		if (asprintf(&bannertoken, "<hr>\n<address>%s</address>\n",
> +		    HTTPD_SERVERNAME) == -1)
> +			return (-1);
> +	}
> +
> +	/* Generate simple HTML error document */
> +	bodylen = asprintf(body,
> +	    "<!DOCTYPE html>\n"
> +	    "<html>\n"
> +	    "<head>\n"
> +	    "<meta charset=\"utf-8\">\n"
> +	    "<title>%03d %s</title>\n"
> +	    "<style type=\"text/css\"><!--\n%s\n--></style>\n"
> +	    "</head>\n"
> +	    "<body>\n"
> +	    "<h1>%03d %s</h1>\n"
> +	    "%s"
> +	    "</body>\n"
> +	    "</html>\n",
> +	    code, httperr, style, code, httperr,
> +	    bannertoken == NULL ? "" : bannertoken);
> +
> +	free(bannertoken);
> +	if (bodylen == -1) {
> +		*body = NULL;
> +		return (-1);
> +	}
> +	return (bodylen);
> +}
> +
>  void
>  server_close_http(struct client *clt)
>  {
> 

-- 
wbr, Kirill