Hello!

On Mon, Jan 23, 2023 at 09:28:42PM +0400, Sergey Kandaurov wrote:

> > On 3 Jan 2023, at 06:48, Maxim Dounin <mdounin@mdounin.ru> wrote:
> >
> > # HG changeset patch
> > # User Maxim Dounin <mdounin@mdounin.ru>
> > # Date 1672713976 -10800
> > # Tue Jan 03 05:46:16 2023 +0300
> > # Node ID e0688b4494f02dcf6feebf0c73e02749bd7de381
> > # Parent 07b0bee87f32be91a33210bc06973e07c4c1dac9
> > Gzip static: ranges support (ticket #2349).
> >
> > In contrast to on-the-fly gzipping with gzip filter, static gzipped
> > representation as returned by gzip_static is persistent, and therefore
> > the same binary representation is available for future requests, making
> > it possible to use range requests.
> >
> > Further, if a gzipped representation is re-generated with different
> > compression settings, it is expected to result in different ETag and
> > different size reported in the Content-Range header, making it possible
> > to safely use range requests anyway.
> >
> > As such, ranges are now allowed for files returned by gzip_static.
> >
> > 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
> > @@ -247,6 +247,8 @@ ngx_http_gzip_static_handler(ngx_http_re
> > ngx_str_set(&h->value, "gzip");
> > r->headers_out.content_encoding = h;
> >
> > + r->allow_ranges = 1;
> > +
> > /* we need to allocate all before the header would be sent */
> >
> > b = ngx_calloc_buf(r->pool);
>
> Looks good.

Thanks for the review, pushed to http://mdounin.ru/hg/nginx.

> 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.

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"

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.

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;
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;


--
Maxim Dounin
http://mdounin.ru/
_______________________________________________
nginx-devel mailing list
nginx-devel@nginx.org
https://mailman.nginx.org/mailman/listinfo/nginx-devel