> On 24 Jan 2023, at 06:19, Maxim Dounin <mdounin@mdounin.ru> wrote:
>
> Hello!
>
> On Mon, Jan 23, 2023 at 09:28:42PM +0400, Sergey Kandaurov wrote:
>
>>> [..]
>
>> On a related note, while comparing with static module, which gzip_static
>> is based on, I further noticed that gzip_static doesn't check for 0-size
>> response in subrequests. Existing handling of r->main suggests that
>> such configuration might be used in practice, e.g. together with gunzip
>> filter, as documented in the gzip_static module documentation.
>> So, it makes sense to add such check for zero size buffers as well.
>>
>> # HG changeset patch
>> # User Sergey Kandaurov <pluknet@nginx.com>
>> # Date 1674493925 -14400
>> # Mon Jan 23 21:12:05 2023 +0400
>> # Node ID 27217fca1966ddb20c843384d438df2af062fdfc
>> # Parent dd458c69858b88231f542be4573a3f81141d1359
>> Gzip static: avoid "zero size buf" alerts in subrequests.
>>
>> Similar to the static module, gzip_static enabled in subrequests might
>> result in zero size buffers with responses from empty precompressed files.
>
> This looks like an issue introduced in 4611:2b6cb7528409, and it
> might be a good idea to fix other affected modules as well.
>
> While valid gzipped responses are not expected to be empty, much
> like valid mp4 and flv files, but it certainly shouldn't cause
> "zero size buf" alerts if there is an invalid file for some
> reason.
>
>> diff --git a/src/http/modules/ngx_http_gzip_static_module.c b/src/http/modules/ngx_http_gzip_static_module.c
>> --- a/src/http/modules/ngx_http_gzip_static_module.c
>> +++ b/src/http/modules/ngx_http_gzip_static_module.c
>> @@ -236,6 +236,10 @@ ngx_http_gzip_static_handler(ngx_http_re
>> return NGX_HTTP_INTERNAL_SERVER_ERROR;
>> }
>>
>> + if (r != r->main && of.size == 0) {
>> + return ngx_http_send_header(r);
>> + }
>> +
>> h = ngx_list_push(&r->headers_out.headers);
>> if (h == NULL) {
>> return NGX_HTTP_INTERNAL_SERVER_ERROR;
>
> While there seems to be no practical difference, I don't think
> that not adding the "Content-Encoding" header for empty subrequest
> responses is a good idea.

I don't see either. Since it's served by the gzip_static handler,
I tend to agree, in a common sense, to always attach this header.

>
> Further, I don't actually think that skipping sending the body, as
> the static module currently does, is a good idea either. It might
> break various filter modules which might not expect such
> behaviour.
>
> I would rather consider sending the body as usual, but with
> b->sync set when sending an empty body in subrequests, similarly
> to how ngx_http_send_special() does. Something like this will do
> the trick:
>
> diff --git a/src/http/modules/ngx_http_gzip_static_module.c b/src/http/modules/ngx_http_gzip_static_module.c
> --- a/src/http/modules/ngx_http_gzip_static_module.c
> +++ b/src/http/modules/ngx_http_gzip_static_module.c
> @@ -273,6 +273,7 @@ ngx_http_gzip_static_handler(ngx_http_re
> b->in_file = b->file_last ? 1 : 0;
> b->last_buf = (r == r->main) ? 1 : 0;
> b->last_in_chain = 1;
> + b->sync = (r == r->main || b->file_last) ? 0 : 1;
>
> b->file->fd = of.fd;
> b->file->name = path;
>
> In particular, with this patch inflate() is able to properly
> report errors on empty files:
>
> 2023/01/24 03:54:44 [error] 22886#100160: *1 inflate() returned -5 on response end while sending response to client, client: 127.0.0.1, server: , request: "GET /index.html HTTP/1.1", subrequest: "/empty.html", host: "127.0.0.1:8080"

Note that if error is returned by any filter in subrequest, such as
with gunzip, this terminates the whole request, due to c->error set.
Well, this behaviour looks good enough, compared to half-baked responses.

>
> I also observe a similar behaviour change with empty xml files
> returned in subrequests by static module and processed by the xslt
> filter: previously, empty xml files were effectively ignored, and
> now they result in the errors much like any other malformed xml
> files.

Well, that confirms that other such places are susceptible.

>
> Full patch:
>
> # HG changeset patch
> # User Maxim Dounin <mdounin@mdounin.ru>
> # Date 1674526244 -10800
> # Tue Jan 24 05:10:44 2023 +0300
> # Node ID 22e41eed77ad95f51d7d0a3daa5cb369c9643697
> # Parent c7e103acb409f0352cb73997c053b3bdbb8dd5db
> Fixed "zero size buf" alerts with subrequests.
>
> Since 4611:2b6cb7528409 responses from the gzip static, flv, and mp4 modules
> can be used with subrequests, though empty files were not properly handled.
> Empty gzipped, flv, and mp4 files thus resulted in "zero size buf in output"
> alerts. While valid corresponding files are not expected to be empty, such
> files shouldn't result in alerts.
>
> Fix is to set b->sync on such empty subrequest responses, similarly to what
> ngx_http_send_special() does.
>
> Additionally, the static module is modified to do the same instead of not
> sending the response body at all in such cases, since not sending the response
> body at all is believed to be at least questionable, and might break various
> filters which do not expect such behaviour.
>
> diff --git a/src/http/modules/ngx_http_flv_module.c b/src/http/modules/ngx_http_flv_module.c
> --- a/src/http/modules/ngx_http_flv_module.c
> +++ b/src/http/modules/ngx_http_flv_module.c
> @@ -235,6 +235,7 @@ ngx_http_flv_handler(ngx_http_request_t
> b->in_file = b->file_last ? 1: 0;

btw, there are old style issues originated in the static module
(it was fixed in gzip_static when it was copied from there)

> b->last_buf = (r == r->main) ? 1 : 0;
> b->last_in_chain = 1;
> + b->sync = (r == r->main || b->file_last) ? 0 : 1;
>
> b->file->fd = of.fd;
> b->file->name = path;
> diff --git a/src/http/modules/ngx_http_gzip_static_module.c b/src/http/modules/ngx_http_gzip_static_module.c
> --- a/src/http/modules/ngx_http_gzip_static_module.c
> +++ b/src/http/modules/ngx_http_gzip_static_module.c
> @@ -273,6 +273,7 @@ ngx_http_gzip_static_handler(ngx_http_re
> b->in_file = b->file_last ? 1 : 0;
> b->last_buf = (r == r->main) ? 1 : 0;
> b->last_in_chain = 1;
> + b->sync = (r == r->main || b->file_last) ? 0 : 1;
>
> b->file->fd = of.fd;
> b->file->name = path;
> diff --git a/src/http/modules/ngx_http_mp4_module.c b/src/http/modules/ngx_http_mp4_module.c
> --- a/src/http/modules/ngx_http_mp4_module.c
> +++ b/src/http/modules/ngx_http_mp4_module.c
> @@ -714,6 +714,7 @@ ngx_http_mp4_handler(ngx_http_request_t
> b->in_file = b->file_last ? 1 : 0;
> b->last_buf = (r == r->main) ? 1 : 0;
> b->last_in_chain = 1;
> + b->sync = (r == r->main || b->file_last) ? 0 : 1;
>
> b->file->fd = of.fd;
> b->file->name = path;
> diff --git a/src/http/modules/ngx_http_static_module.c b/src/http/modules/ngx_http_static_module.c
> --- a/src/http/modules/ngx_http_static_module.c
> +++ b/src/http/modules/ngx_http_static_module.c
> @@ -238,10 +238,6 @@ ngx_http_static_handler(ngx_http_request
> return NGX_HTTP_INTERNAL_SERVER_ERROR;
> }
>
> - if (r != r->main && of.size == 0) {
> - return ngx_http_send_header(r);
> - }
> -
> r->allow_ranges = 1;
>
> /* we need to allocate all before the header would be sent */
> @@ -268,6 +264,7 @@ ngx_http_static_handler(ngx_http_request
> b->in_file = b->file_last ? 1: 0;
> b->last_buf = (r == r->main) ? 1: 0;
> b->last_in_chain = 1;
> + b->sync = (r == r->main || b->file_last) ? 0 : 1;
>
> b->file->fd = of.fd;
> b->file->name = path;
>

In addition to the static module change, any reason not to include
ngx_http_cache_send()? It has a similar behaviour to skip cached
empty body if served in subrequests. Yet another similar place is
subrequests processed with rewrite such as "return 204;".

diff --git a/src/http/ngx_http_core_module.c b/src/http/ngx_http_core_module.c
--- a/src/http/ngx_http_core_module.c
+++ b/src/http/ngx_http_core_module.c
@@ -1803,10 +1803,6 @@ ngx_http_send_response(ngx_http_request_
}
}

- if (r != r->main && val.len == 0) {
- return ngx_http_send_header(r);
- }
-
b = ngx_calloc_buf(r->pool);
if (b == NULL) {
return NGX_HTTP_INTERNAL_SERVER_ERROR;
@@ -1817,6 +1813,7 @@ ngx_http_send_response(ngx_http_request_
b->memory = val.len ? 1 : 0;
b->last_buf = (r == r->main) ? 1 : 0;
b->last_in_chain = 1;
+ b->sync = (r == r->main || val.len) ? 0 : 1;

out.buf = b;
out.next = NULL;
diff --git a/src/http/ngx_http_file_cache.c b/src/http/ngx_http_file_cache.c
--- a/src/http/ngx_http_file_cache.c
+++ b/src/http/ngx_http_file_cache.c
@@ -1575,10 +1575,6 @@ ngx_http_cache_send(ngx_http_request_t *
ngx_log_debug1(NGX_LOG_DEBUG_HTTP, r->connection->log, 0,
"http file cache send: %s", c->file.name.data);

- if (r != r->main && c->length - c->body_start == 0) {
- return ngx_http_send_header(r);
- }
-
/* we need to allocate all before the header would be sent */

b = ngx_calloc_buf(r->pool);
@@ -1603,6 +1599,7 @@ ngx_http_cache_send(ngx_http_request_t *
b->in_file = (c->length - c->body_start) ? 1: 0;
b->last_buf = (r == r->main) ? 1: 0;
b->last_in_chain = 1;
+ b->sync = (r == r->main || b->file_last - b->file_pos) ? 0 : 1;

b->file->fd = c->file.fd;
b->file->name = c->file.name;

--
Sergey Kandaurov
_______________________________________________
nginx-devel mailing list
nginx-devel@nginx.org
https://mailman.nginx.org/mailman/listinfo/nginx-devel