From: Rafael Sadowski Subject: httpd: drain abort response via bufferevent To: tech@openbsd.org Cc: "Kirill A. Korinsky" , Claudio Jeker Date: Thu, 25 Jun 2026 13:03:33 +0200 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: 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, "
\n
%s
\n", - HTTPD_SERVERNAME) == -1) { - bannertoken = NULL; - goto done; - } - - /* Generate simple HTML error document */ - if ((bodylen = asprintf(&body, - "\n" - "\n" - "\n" - "\n" - "%03d %s\n" - "\n" - "\n" - "\n" - "

%03d %s

\n" - "%s" - "\n" - "\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, "
\n
%s
\n", + HTTPD_SERVERNAME) == -1) + return (-1); + } + + /* Generate simple HTML error document */ + bodylen = asprintf(body, + "\n" + "\n" + "\n" + "\n" + "%03d %s\n" + "\n" + "\n" + "\n" + "

%03d %s

\n" + "%s" + "\n" + "\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) {