On 2022/08/11 19:37:08 +0200, Claudio Jeker wrote:
> Reading through the fcgi code of httpd I wonder if HEAD requests are
> handled correctly. In server_fcgi_read() the function just returns when it
> hits the HEAD case but I think this is not correct since it does not drain
> the clt_srvevb buffer and it kind of ignores possible padding bytes.
FWIW I was wondering the same but I think it's fine to `return' here.
It's a small optimization: if we've reached FCGI_END_REQUEST there is
no more to read and padding is very likely zero anyway. clt_srvevb is
re-created on the next server_fcgi.
However, looking into this made me realize the diff I committed a few
hours ago has a regression with HEAD requests: it prints the end
marker when it shouldn't.
> I think what the code should do is break out of the switch case and just
> continue. This is still not quite right since httpd depends on the fcgi
> server to close the connection (or it will do that on the next
> sever_fcgi() call).
diff /usr/src
commit - acc148801b858e3177b9b43a93d20fd53f97d40d
path + /usr/src
blob - b6541b7c68235ac1dfc5a9d0243db988e5932a7f
file + usr.sbin/httpd/server_fcgi.c
--- usr.sbin/httpd/server_fcgi.c
+++ usr.sbin/httpd/server_fcgi.c
@@ -486,6 +486,7 @@ void
server_fcgi_error(struct bufferevent *bev, short error, void *arg)
{
struct client *clt = arg;
+ struct http_descriptor *desc = clt->clt_descreq;
if ((error & EVBUFFER_EOF) && !clt->clt_fcgi.headersdone) {
server_abort_http(clt, 500, "malformed or no headers");
@@ -493,7 +494,8 @@ server_fcgi_error(struct bufferevent *bev, short error
}
/* send the end marker if not already */
- if (clt->clt_fcgi.chunked && !clt->clt_fcgi.end++)
+ if (desc->http_method != HTTP_METHOD_HEAD && clt->clt_fcgi.chunked &&
+ !clt->clt_fcgi.end++)
server_bufferevent_print(clt, "0\r\n\r\n");
server_file_error(bev, error, arg);
