Hello!

On Wed, Jan 25, 2023 at 01:50:56PM +0400, Sergey Kandaurov wrote:

> > On 24 Jan 2023, at 06:19, Maxim Dounin <mdounin@mdounin.ru> wrote:
> >
> > 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)

This is actually an (old) style, which was previously used in many
places. Now it remains mostly in multi-line forms of the
conditional operator, e.g.:

r->loc_conf = (node->exact) ? node->exact->loc_conf:
node->inclusive->loc_conf;

Single-line form is indeed mostly unused now, and probably it's
time to remove remaining occurrences. Added a patch for this.

[...]

> > @@ -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;".

Indeed, thanks for catching, missed these two.

Also, looking into this places I tend to think it's better to use

b->sync = (b->last_buf || b->in_file) ? 0 : 1;

instead, which is going to be simpler and more universal across
all uses.

# HG changeset patch
# User Maxim Dounin <mdounin@mdounin.ru>
# Date 1674872423 -10800
# Sat Jan 28 05:20:23 2023 +0300
# Node ID 1c3b78d7cdc9e7059e90aa1670d58fd37927a1a2
# Parent 106328a70f4ecb32f828d33e5cd66c861e455f92
Style.

diff --git a/src/event/ngx_event_connectex.c b/src/event/ngx_event_connectex.c
--- a/src/event/ngx_event_connectex.c
+++ b/src/event/ngx_event_connectex.c
@@ -127,8 +127,8 @@ void ngx_iocp_wait_events(int main)
conn[0] = NULL;

for ( ;; ) {
- offset = (nevents == WSA_MAXIMUM_WAIT_EVENTS + 1) ? 1: 0;
- timeout = (nevents == 1 && !first) ? 60000: INFINITE;
+ offset = (nevents == WSA_MAXIMUM_WAIT_EVENTS + 1) ? 1 : 0;
+ timeout = (nevents == 1 && !first) ? 60000 : INFINITE;

n = WSAWaitForMultipleEvents(nevents - offset, events[offset],
0, timeout, 0);
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
@@ -232,7 +232,7 @@ ngx_http_flv_handler(ngx_http_request_t
b->file_pos = start;
b->file_last = of.size;

- b->in_file = b->file_last ? 1: 0;
+ b->in_file = b->file_last ? 1 : 0;
b->last_buf = (r == r->main) ? 1 : 0;
b->last_in_chain = 1;

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
@@ -265,8 +265,8 @@ ngx_http_static_handler(ngx_http_request
b->file_pos = 0;
b->file_last = of.size;

- b->in_file = b->file_last ? 1: 0;
- b->last_buf = (r == r->main) ? 1: 0;
+ b->in_file = b->file_last ? 1 : 0;
+ b->last_buf = (r == r->main) ? 1 : 0;
b->last_in_chain = 1;

b->file->fd = of.fd;
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
@@ -1600,8 +1600,8 @@ ngx_http_cache_send(ngx_http_request_t *
b->file_pos = c->body_start;
b->file_last = c->length;

- b->in_file = (c->length - c->body_start) ? 1: 0;
- b->last_buf = (r == r->main) ? 1: 0;
+ b->in_file = (c->length - c->body_start) ? 1 : 0;
+ b->last_buf = (r == r->main) ? 1 : 0;
b->last_in_chain = 1;

b->file->fd = c->file.fd;
# HG changeset patch
# User Maxim Dounin <mdounin@mdounin.ru>
# Date 1674872613 -10800
# Sat Jan 28 05:23:33 2023 +0300
# Node ID f5515e727656b2cc529b26a0fe6b4496c62e32b2
# Parent 1c3b78d7cdc9e7059e90aa1670d58fd37927a1a2
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, the ngx_http_send_response() function, and
file cache are 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 = (b->last_buf || b->in_file) ? 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 = (b->last_buf || b->in_file) ? 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 = (b->last_buf || b->in_file) ? 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 = (b->last_buf || b->in_file) ? 0 : 1;

b->file->fd = of.fd;
b->file->name = path;
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 = (b->last_buf || b->memory) ? 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 = (b->last_buf || b->in_file) ? 0 : 1;

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

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