> On 24 Mar 2023, at 06:07, Maxim Dounin <mdounin@mdounin.ru> wrote:
>
> Hello!
>
> On Thu, Mar 23, 2023 at 09:33:19PM +0100, Richard Stanway via nginx wrote:
>
>> Yes, when using the latest zlib-ng on nginx-1.21.6 I received the
>> alerts. Previous versions of zlib-ng have worked great after the 2021
>> patch. I tried to update it myself as follows based on advice of
>> zlib-ng GitHub issues, while it reduced the number of alerts logged it
>> did not completely solve the issue so it seems the memory requirements
>> may have further changed. While I would appreciate a proper patch
>> making it into nginx, the seemingly-frequent upstream changes may make
>> this difficult to maintain.
>>
>> - ctx->allocated = 8192 + 16 + (1 << (wbits + 2))
>> + ctx->allocated = 8192 + 288 + 16 + (1 << (wbits + 2))
>> + 131072 + (1 << (memlevel + 8));
>
> It looks like there are at least two changes in zlib-ng since I
> looked into it:
>
> - Window bits are no longer forced to 13 on compression level 1.
>
> - All allocations use custom alloc_aligned() wrapper, and
> therefore all allocations are larger than expected by (64 +
> sizeof(void*)).
>
> Further, due to the wrapper nginx sees all allocations as an
> allocation of 1 element of a given size, so it misinterprets
> some allocations as the state allocation.
>
> [..]
>
> Please try the following patch, it should help with recent versions:
>
> # HG changeset patch
> # User Maxim Dounin <mdounin@mdounin.ru>
> # Date 1679622670 -10800
> # Fri Mar 24 04:51:10 2023 +0300
> # Node ID 67a0999550c3622e51639acb8bde57d199826f7e
> # Parent d1cf09451ae84b930ce66fa6d63ae3f7eeeac5a5
> Gzip: compatibility with recent zlib-ng versions.
>
> It now uses custom alloc_aligned() wrapper for all allocations,
> therefore all allocations are larger than expected by (64 + sizeof(void*)).
> Further, they are seen as allocations of 1 element. Relevant calculations
> were adjusted to reflect this, and state allocation is now protected
> with a flag to avoid misinterpreting other allocations as the zlib
> deflate_state allocation.
>
> Further, it no longer forces window bits to 13 on compression level 1,
> so the comment was adjusted to reflect this.

For the record, the corresponding zlib-ng git commits:
ce6789c7e093e8e6bb6fc591bbdf0f805999bdb9
a39e323a4db80a57feecf2ae212c08070234050c

>
> diff --git a/src/http/modules/ngx_http_gzip_filter_module.c b/src/http/modules/ngx_http_gzip_filter_module.c
> --- a/src/http/modules/ngx_http_gzip_filter_module.c
> +++ b/src/http/modules/ngx_http_gzip_filter_module.c
> @@ -57,6 +57,7 @@ typedef struct {
> unsigned nomem:1;
> unsigned buffering:1;
> unsigned zlib_ng:1;
> + unsigned state_allocated:1;
>
> size_t zin;
> size_t zout;
> @@ -514,9 +515,10 @@ ngx_http_gzip_filter_memory(ngx_http_req
> } else {
> /*
> * Another zlib variant, https://github.com/zlib-ng/zlib-ng.
> - * It forces window bits to 13 for fast compression level,
> - * uses 16-byte padding in one of window-sized buffers, and
> - * uses 128K hash.
> + * It used to force window bits to 13 for fast compression level,

BTW, it makes sense to peel off this extra allocation somewhere in future,
similar to how it was done for updated handling of zlib variant from Intel.

> + * uses (64 + sizeof(void*)) additional space on all allocations
> + * for alignment, 16-byte padding in one of window-sized buffers,
> + * and 128K hash.
> */
>
> if (conf->level == 1) {
> @@ -524,7 +526,8 @@ ngx_http_gzip_filter_memory(ngx_http_req
> }
>
> ctx->allocated = 8192 + 16 + (1 << (wbits + 2))
> - + 131072 + (1 << (memlevel + 8));
> + + 131072 + (1 << (memlevel + 8))
> + + 4 * (64 + sizeof(void*));
> ctx->zlib_ng = 1;
> }
> }
> @@ -926,13 +929,16 @@ ngx_http_gzip_filter_alloc(void *opaque,
>
> alloc = items * size;
>
> - if (items == 1 && alloc % 512 != 0 && alloc < 8192) {
> -
> + if (items == 1 && alloc % 512 != 0 && alloc < 8192
> + && !ctx->state_allocated)
> + {
> /*
> * The zlib deflate_state allocation, it takes about 6K,
> * we allocate 8K. Other allocations are divisible by 512.
> */
>
> + ctx->state_allocated = 1;
> +
> alloc = 8192;

Nitpicking:
now the allocation size comment and assignment look separated.
Mind moving the ctx->state_allocated line below?

> }
>

Looks good otherwise.

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