Re: httpd(8): don't try to chunk-encode an empty body
On 2021-05-18 00:47 +02, Sebastian Benoit wrote: > The comments in server_fcgi_header seem to suggest more dragons lurk in this > area. Sush! -- I'm not entirely sure you are real.
Re: httpd(8): don't try to chunk-encode an empty body
Florian Obser(flor...@openbsd.org) on 2021.05.15 17:14:38 +0200:
> Turns out it's not that difficult to do this correctly since we already
> wait until we read all http headers from the fcgi upstream. We just need
> to delay writing of the http header until we know if the body is empty
> or not.
>
> OK?
seems to work, ok benno@
The comments in server_fcgi_header seem to suggest more dragons lurk in this
area.
>
>
> diff --git httpd.h httpd.h
> index b3a40b3af68..c4adfba232d 100644
> --- httpd.h
> +++ httpd.h
> @@ -300,6 +300,7 @@ struct fcgi_data {
> int end;
> int status;
> int headersdone;
> + int headerssent;
> };
>
> struct range {
> diff --git server_fcgi.c server_fcgi.c
> index 64a0e9d2abb..e1e9704c920 100644
> --- server_fcgi.c
> +++ server_fcgi.c
> @@ -114,6 +114,7 @@ server_fcgi(struct httpd *env, struct client *clt)
> clt->clt_fcgi.toread = sizeof(struct fcgi_record_header);
> clt->clt_fcgi.status = 200;
> clt->clt_fcgi.headersdone = 0;
> + clt->clt_fcgi.headerssent = 0;
>
> if (clt->clt_srvevb != NULL)
> evbuffer_free(clt->clt_srvevb);
> @@ -544,22 +545,20 @@ server_fcgi_read(struct bufferevent *bev, void *arg)
> if (!clt->clt_fcgi.headersdone) {
> clt->clt_fcgi.headersdone =
> server_fcgi_getheaders(clt);
> - if (clt->clt_fcgi.headersdone) {
> - if (server_fcgi_header(clt,
> - clt->clt_fcgi.status)
> - == -1) {
> - server_abort_http(clt,
> - 500,
> - "malformed fcgi "
> - "headers");
> - return;
> - }
> - }
> if (!EVBUFFER_LENGTH(clt->clt_srvevb))
> break;
> }
> /* FALLTHROUGH */
> case FCGI_END_REQUEST:
> + if (clt->clt_fcgi.headersdone &&
> + !clt->clt_fcgi.headerssent) {
> + if (server_fcgi_header(clt,
> + clt->clt_fcgi.status) == -1) {
> + server_abort_http(clt, 500,
> + "malformed fcgi headers");
> + return;
> + }
> + }
> if (server_fcgi_writechunk(clt) == -1) {
> server_abort_http(clt, 500,
> "encoding error");
> @@ -600,6 +599,8 @@ server_fcgi_header(struct client *clt, unsigned int code)
> char tmbuf[32];
> struct kv *kv, *cl, key;
>
> + clt->clt_fcgi.headerssent = 1;
> +
> if (desc == NULL || (error = server_httperror_byid(code)) == NULL)
> return (-1);
>
> @@ -615,6 +616,12 @@ server_fcgi_header(struct client *clt, unsigned int code)
> if (kv_add(>http_headers, "Server", HTTPD_SERVERNAME) == NULL)
> return (-1);
>
> + if (clt->clt_fcgi.type == FCGI_END_REQUEST ||
> + EVBUFFER_LENGTH(clt->clt_srvevb) == 0) {
> + /* Can't chunk encode an empty body. */
> + clt->clt_fcgi.chunked = 0;
> + }
> +
> /* Set chunked encoding */
> if (clt->clt_fcgi.chunked) {
> /* XXX Should we keep and handle Content-Length instead? */
>
>
> --
> I'm not entirely sure you are real.
>
Re: httpd(8): don't try to chunk-encode an empty body
Hi,
I can confirm this patch (from the 6.9 source) makes httpd work with
Android Nextcloud client 3.16, where the unpatched httpd does not work.
Thanks very much!
Cheers,
Steve Williams
On 15/05/2021 9:14 a.m., Florian Obser wrote:
Turns out it's not that difficult to do this correctly since we already
wait until we read all http headers from the fcgi upstream. We just need
to delay writing of the http header until we know if the body is empty
or not.
OK?
diff --git httpd.h httpd.h
index b3a40b3af68..c4adfba232d 100644
--- httpd.h
+++ httpd.h
@@ -300,6 +300,7 @@ struct fcgi_data {
int end;
int status;
int headersdone;
+ int headerssent;
};
struct range {
diff --git server_fcgi.c server_fcgi.c
index 64a0e9d2abb..e1e9704c920 100644
--- server_fcgi.c
+++ server_fcgi.c
@@ -114,6 +114,7 @@ server_fcgi(struct httpd *env, struct client *clt)
clt->clt_fcgi.toread = sizeof(struct fcgi_record_header);
clt->clt_fcgi.status = 200;
clt->clt_fcgi.headersdone = 0;
+ clt->clt_fcgi.headerssent = 0;
if (clt->clt_srvevb != NULL)
evbuffer_free(clt->clt_srvevb);
@@ -544,22 +545,20 @@ server_fcgi_read(struct bufferevent *bev, void *arg)
if (!clt->clt_fcgi.headersdone) {
clt->clt_fcgi.headersdone =
server_fcgi_getheaders(clt);
- if (clt->clt_fcgi.headersdone) {
- if (server_fcgi_header(clt,
- clt->clt_fcgi.status)
- == -1) {
- server_abort_http(clt,
- 500,
- "malformed fcgi "
- "headers");
- return;
- }
- }
if (!EVBUFFER_LENGTH(clt->clt_srvevb))
break;
}
/* FALLTHROUGH */
case FCGI_END_REQUEST:
+ if (clt->clt_fcgi.headersdone &&
+ !clt->clt_fcgi.headerssent) {
+ if (server_fcgi_header(clt,
+ clt->clt_fcgi.status) == -1) {
+ server_abort_http(clt, 500,
+ "malformed fcgi headers");
+ return;
+ }
+ }
if (server_fcgi_writechunk(clt) == -1) {
server_abort_http(clt, 500,
"encoding error");
@@ -600,6 +599,8 @@ server_fcgi_header(struct client *clt, unsigned int code)
char tmbuf[32];
struct kv *kv, *cl, key;
+ clt->clt_fcgi.headerssent = 1;
+
if (desc == NULL || (error = server_httperror_byid(code)) == NULL)
return (-1);
@@ -615,6 +616,12 @@ server_fcgi_header(struct client *clt, unsigned int code)
if (kv_add(>http_headers, "Server", HTTPD_SERVERNAME) == NULL)
return (-1);
+ if (clt->clt_fcgi.type == FCGI_END_REQUEST ||
+ EVBUFFER_LENGTH(clt->clt_srvevb) == 0) {
+ /* Can't chunk encode an empty body. */
+ clt->clt_fcgi.chunked = 0;
+ }
+
/* Set chunked encoding */
if (clt->clt_fcgi.chunked) {
/* XXX Should we keep and handle Content-Length instead? */
Re: httpd(8): don't try to chunk-encode an empty body
Turns out it's not that difficult to do this correctly since we already
wait until we read all http headers from the fcgi upstream. We just need
to delay writing of the http header until we know if the body is empty
or not.
OK?
diff --git httpd.h httpd.h
index b3a40b3af68..c4adfba232d 100644
--- httpd.h
+++ httpd.h
@@ -300,6 +300,7 @@ struct fcgi_data {
int end;
int status;
int headersdone;
+ int headerssent;
};
struct range {
diff --git server_fcgi.c server_fcgi.c
index 64a0e9d2abb..e1e9704c920 100644
--- server_fcgi.c
+++ server_fcgi.c
@@ -114,6 +114,7 @@ server_fcgi(struct httpd *env, struct client *clt)
clt->clt_fcgi.toread = sizeof(struct fcgi_record_header);
clt->clt_fcgi.status = 200;
clt->clt_fcgi.headersdone = 0;
+ clt->clt_fcgi.headerssent = 0;
if (clt->clt_srvevb != NULL)
evbuffer_free(clt->clt_srvevb);
@@ -544,22 +545,20 @@ server_fcgi_read(struct bufferevent *bev, void *arg)
if (!clt->clt_fcgi.headersdone) {
clt->clt_fcgi.headersdone =
server_fcgi_getheaders(clt);
- if (clt->clt_fcgi.headersdone) {
- if (server_fcgi_header(clt,
- clt->clt_fcgi.status)
- == -1) {
- server_abort_http(clt,
- 500,
- "malformed fcgi "
- "headers");
- return;
- }
- }
if (!EVBUFFER_LENGTH(clt->clt_srvevb))
break;
}
/* FALLTHROUGH */
case FCGI_END_REQUEST:
+ if (clt->clt_fcgi.headersdone &&
+ !clt->clt_fcgi.headerssent) {
+ if (server_fcgi_header(clt,
+ clt->clt_fcgi.status) == -1) {
+ server_abort_http(clt, 500,
+ "malformed fcgi headers");
+ return;
+ }
+ }
if (server_fcgi_writechunk(clt) == -1) {
server_abort_http(clt, 500,
"encoding error");
@@ -600,6 +599,8 @@ server_fcgi_header(struct client *clt, unsigned int code)
char tmbuf[32];
struct kv *kv, *cl, key;
+ clt->clt_fcgi.headerssent = 1;
+
if (desc == NULL || (error = server_httperror_byid(code)) == NULL)
return (-1);
@@ -615,6 +616,12 @@ server_fcgi_header(struct client *clt, unsigned int code)
if (kv_add(>http_headers, "Server", HTTPD_SERVERNAME) == NULL)
return (-1);
+ if (clt->clt_fcgi.type == FCGI_END_REQUEST ||
+ EVBUFFER_LENGTH(clt->clt_srvevb) == 0) {
+ /* Can't chunk encode an empty body. */
+ clt->clt_fcgi.chunked = 0;
+ }
+
/* Set chunked encoding */
if (clt->clt_fcgi.chunked) {
/* XXX Should we keep and handle Content-Length instead? */
--
I'm not entirely sure you are real.
Re: httpd(8): don't try to chunk-encode an empty body
Florian Obser(flor...@openbsd.org) on 2021.05.14 19:13:49 +0200:
> As found out by Chris Narkiewicz the hard way, trying to chunk encode an
> empty body makes the nextclown app stop working. (see "Nextcloud stopped
> working after upgrade to 6.9" on ports@).
>
> I don't think there is a valid way to do this, so don't try to.
>
> This is kinda maybe a hack since there might be other stuff that isn't
> sending a body. So if anyone has a long list of status codes that should
> be in the same list, let me know.
https://datatracker.ietf.org/doc/html/rfc7230#page-32
gives the list as
... any response with a 1xx
(Informational), 204 (No Content), or 304 (Not Modified) status
code is always terminated by the first empty line after the
header fields, regardless of the header fields present in the
message, and thus cannot contain a message body.
see the text for other responses that must have an empty body (for other
reasons than the status code).
/Benno
>
> We can't easily find out if the fcgi server is going to send us an
> empty body without reading everything from the server until we hit the
> body. I'm happy to entertain a diff that does that.
>
> In the meantime, this scratches my itch.
> OK?
ok benno@
>
> diff --git server_fcgi.c server_fcgi.c
> index 8d3b581568f..0ac80c27d11 100644
> --- server_fcgi.c
> +++ server_fcgi.c
> @@ -615,6 +615,10 @@ server_fcgi_header(struct client *clt, unsigned int code)
> if (kv_add(>http_headers, "Server", HTTPD_SERVERNAME) == NULL)
> return (-1);
>
> + /* we cannot chunk-encode no-content */
> + if (code == 204)
> + clt->clt_fcgi.chunked = 0;
> +
> /* Set chunked encoding */
> if (clt->clt_fcgi.chunked) {
> /* XXX Should we keep and handle Content-Length instead? */
>
>
> --
> I'm not entirely sure you are real.
>
