[PATCH] Gzip static: ranges support (ticket #2349)

Sergey Kandaurov pluknet at nginx.com
Wed Jan 25 09:50:56 UTC 2023


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


More information about the nginx-devel mailing list